Phoenix: Provide integration with Elixir v1.6 formatter

[josevalim]

This can be broken in 4 separate tasks:

• [x] Add a .formatter.exs to Phoenix itself so we can use import_deps
• [x] Add a .formatter.exs to generated phx.new projects and make sure the code generated by default is formatted
• [x] Add a .formatter.exs to generate phx.new --umbrella projects and make sure the code generated by default is formatted. We need to use the format defined here.
• [ ] Make sure that code generated by phx.gen.html and phx.gen.json are formatted by default

For 2 and 3, we can check the generated code is formatted like this. Separate PRs would make this easier to review.

I also recommend us to enable the :trim option in our EEx code generators, as that will make whitespace much easier to manage.

[michallepicki]

As elixir approaches 1.6.0 I tried the formatter on a default phx.new project. I also looked at one of my projects. At first glance, questionable parens:

• socket, recv
• plug, pipe_through, forward, match, resources
• get, post, put, delete
• render, json, text, html
• Ecto's add, create, create_if_not_exists, drop, drop_if_exists, execute, modify, remove, rename

[chrismccord]

Yes we will need special formatter config for new projects for DSL macros such as the router

[josevalim]

@michallepicki a PR is definitely appreciated. I don't think render, json, text and html should make the cut though. I would mostly restrict to module-level DSLs and testing concepts.

The ones about Ecto should be added to Ecto. A PR is welcome there too!

[michallepicki]

Sure, I'm on it.

[michallepicki]

I compiled an initial list of functions here. I followed the Phoenix Docs and the way all of the functions were written there (I skipped some cases where I assumed the Docs just didn't get updated).

1. I am still unsure about whether we shouldn't also include Phoenix.Controller's html etc, Phoenix.View's renders, Phoenix.ChannelTest and Phoenix.ConnTest replicas of socket, broadcast, get, put (but in this case we already have some of them on the list and only arity may sometimes differ so I can include these).
2. I am generally (a little bit) disappointed with the formatter and the fact that I can't specify an exact function by providing the full name with package and module, e.g. "Phoenix.Router.forward": 2 because of possible function name overlaps.
3. The things that (I assume) are left to do:
   
   
   
   • Do the same in Ecto and Plug
   
   
   • Generate a .formatter.exs with phx.new which imports appropriate settings from :ecto, :phoenix depending on task arguments
   
   

I assume formatting the codebase and making the docs consistent should be done in other issues.

[chrismccord]

1. We should paren all function calls, like render and broadcast. The router, channel/socket macros should be excluded (get, post, channel), and friends.

2. will defer to José

3. Precisely, please proceed!

Formatting the codebase and docs will be happening once the dust settles on other areas. I'm not sure yet if it will be done on an ongoing basis or at once, but one step at a time. Thanks!

[josevalim]

1. I agree with Chris. I have sent a PR here with some notes: https://github.com/michallepicki/phoenix/pull/1

2. The formatter does not have semantic understanding of the code. In order to have that, it would have to execute code. So when the formatter sees get "/foo", Bar, :baz, it has no idea where get came from, especially because get was imported by something behind a use call, which we can just know what it is if the code was compiled and executed. That would be quite hard, or even impossible, and definitely very slow.

3. In two parts:

   a. It may not be necessary for Phoenix itself since Phoenix does not really use any of the Plug DSLs. It does use plug/2 but that we can define ourselves? Ecto would definitely be welcome though.

   b. Yes! Here is the one Mix generates. You may have to adjust the inputs to include "priv/repo" directory (if using ecto).

[michallepicki]

@josevalim A bit off-topic but more about my concerns:
Don't we need a way to exclude (or apply only for some files) parts of the formatter settings imported into a project? Let's say I have a phoenix project where I have my own options/3 function somewhere unrelated to the Phoenix.Router, and it should have parentheses. Right now I don't see a way to have both Phoenix.Router.options/3 without parentheses and my own options/3 with parentheses. I'd even settle for optional group of settings per input: file pattern, like

[
  inputs: [
    { "lib/**/router.ex", import_deps: [:phoenix] },
    "lib/**/*.{ex,exs}"
  ]
]

They'd match top to bottom.
Edit: Or even that combined with export groups, like a :phoenix_routing_dsl group. One could either import whole :phoenix which would apply all groups or only :phoenix_routing_dsl settings into his router module

[josevalim]

That was actually one of my concerns with the import_deps stuff. They are too general and it is more likely you will end-up with false positives like above.

Maybe a good course of action for now is to only add get/put/post/delete/match from the router and document in Phoenix's own .formatter that the other ones were skipped because few teams use them. They can always add it to their own .formatter.exs.

[fertapric]

Other option could be to provide several formatter blocks in the .formatter.exs file. All the blocks would follow the same options as the ones currently supported by the formatter:

[
  [
    inputs: [
      "lib/**/router.ex"
    ],
    import_deps: [:phoenix]
  ],
  [
    inputs: [
      "lib/**/*.{ex,exs}"
    ],
    locals_without_parens: [
      my_function: 2
    ]
  ]
]

However, I would keep compatibility with the current one-block format by converting it internally to a list of one item. Regarding :export, I guess I would merge all the conditions into a single one or move it outside the blocks.

[michallepicki]

I feel like this discussion should be moved somewhere else

edit: we can continue this discussion on the mailing list here: https://groups.google.com/d/topic/elixir-lang-core/xTduV3KZvfY/discussion

[fertapric]

Maybe a good course of action for now is to only add get/put/post/delete/match from the router and document in Phoenix's own .formatter that the other ones were skipped because few teams use them. They can always add it to their own .formatter.exs.

Is get going to collide with Map.get/3 or Agent.get/3, put with Keyword.put/3 or Map.put/3, or delete with Keyword.delete/3?

[josevalim]

If you import them, yes. But it is not common at all to have those functions imported.

[fertapric]

I'm currently working on "Add a .formatter.exs to generated phx.new projects and make sure the code generated by default is formatted". The hardest part is to check that the generated code by default is formatted.

This is how the tests look like at the moment:

# Formatter
assert_file "phx_blog/.formatter.exs", fn file ->
  assert file =~ "inputs: [\"{mix,.formatter}.exs\", \"{config,lib,test}/**/*.{ex,exs}\"],"
  assert file =~ "import_deps: [:ecto, :phoenix]"
end

# Ensure formatting is setup and consistent.
Mix.Project.in_project :phx_blog, "phx_blog", fn _module ->
  Mix.Tasks.Deps.Get.run([])
  Mix.Tasks.Format.run(["--check-formatted"])
end

As you can see, I need to download the dependencies so I can use import_deps in the .formatter.exs. Otherwise, it raises:

** (Mix.Error) Unknown dependency :ecto given to :import_deps in the formatter configuration. The dependency is not listed in your mix.exs file

If I download the dependencies first, it works as expected but I'm a bit concerned about the idea of requiring an Internet connection to run the test suite. I have thought about moving these tests to different test cases and tag them. What do you think?

In addition, I had some 🤔 while trying to make the tests pass:

• the formatter configuration that is used to check the generated code it's the one included in the latest release and not the one at the root of the repo. I'm ok with this, but I thought it would be nice to point this out as it might be confusing for the people working on the repo.
• the latest version of Phoenix v1.3.2 does not include the formatter configuration, and I cannot adapt the templates to the proper format. Would be possible to release a new version with the .formatter.exs file? Otherwise, I could temporary replace import_deps: [:phoenix] with the locals_without_parens of the .formatter.exs at the root of the repo.
• there are some templates that are not so easy to adapt. Here is an example: https://github.com/phoenixframework/phoenix/blob/v1.3.2/installer/templates/phx_single/config/config.exs#L9-L12. Depending on the flags (and I guess the length of the name of the project), the formatter could require a one-line or a multi-line expression:

-config :hello,
-  ecto_repos: [Hello.Repo]
+config :hello, ecto_repos: [Hello.Repo]

• some tests that relied on one-line expressions must be converted to multi-line tests https://github.com/phoenixframework/phoenix/blob/v1.3.2/installer/test/phx_new_test.exs#L49

assert_file "phx_blog/lib/phx_blog_web.ex", fn file ->
  assert file =~ "defmodule PhxBlogWeb do"
  assert file =~ "use Phoenix.View,"
  assert file =~ "  root: \"lib/phx_blog_web/templates\""
end

• the transport macro will include parens, because it seems it's going away on v1.4

[fertapric]

the formatter configuration that is used to check the generated code it's the one included in the latest release and not the one at the root of the repo. I'm ok with this, but I thought it would be nice to point this out as it might be confusing for the people working on the repo.

It's also worth mentioning another drawback: new releases with changes in the configuration of the formatter could potentially break the tests of the installer.

[josevalim]

@fertapric we definitely shouldn't fetch the deps while running tests.

For this reason, the only way we can run the formatter as in the phoenix project tests themselves and not inside the installer. For example, here is a test that guarantees the application compiles: https://github.com/phoenixframework/phoenix/blob/master/test/mix/tasks/phx.new_test.exs. Hopefully we are capable of adding the formatter tests there.

This will also help us manage the other drawback you mentioned, as we will leverage Phoenix' mix.lock.

[michallepicki]

@josevalim We indeed can --check-formatted on the generated code in phoenix project tests just like we're checking if it compiles, and I think we could somehow work out importing phoenix's formatter settings without getting deps (since the file is in the same repo), but I don't see a solution for importing ecto's formatter settings. So I think we wouldn't be able to have such tests for all project templates. Or we'd have to skip files that include ecto code.

So I think the "make sure the code generated by default is formatted" part can be done only partially... We could add a travis pipeline check though... Any thoughts?

cc @fertapric

[josevalim]

@michallepicki I see. We can maybe add ecto as a test dependency? Which is what we do for gettext and phoenix_html? Or alternatively edit the .formatter.exs file to not include Ecto since none of the Ecto DSL should affect a generate project?

[tomciopp]

It looks like this is the last issue before cutting a 1.4 release of phoenix. Is there any way to contribute so that we can get a new version out the door?

[michallepicki]

@tomciopp I believe the tasks outlined in the issue description are still valid.

I dealt with most of

Add a .formatter.exs to Phoenix itself so we can use import_deps

But the .formatter.exs file I added probably could be improved further. You can try it out by using Phoenix master in your project. Change

 {:phoenix, "~> 1.3.4"},

to

  {:phoenix, github: "phoenixframework/phoenix", override: true},

in your mix.exs file, run mix deps.get, add

  import_deps: [:phoenix]

to your project's .formatter.exs file (don't forget to include formatter inputs, if you're creating a new .formatter.exs file), and run mix format.

If you want to help with the remaining tasks from the issue description, please read the above discussion and feel free to contribute with ideas, solutions and PRs regarding these.

[aptinio]

I've taken a stab (#3033) at:

Add a .formatter.exs to generated phx.new projects and make sure the code generated by default is formatted

It's a WIP. I'd appreciate your feedback and guidance.

[mikeover]

Gave the updates for umbrella projects a shot at https://github.com/phoenixframework/phoenix/pull/3035. Not sure if I got everything as I expected there to be more things to update so would appreciate some 👀.

[mikeover]

Think you can check off that 3rd bullet point now 👍

[sublimecoder]

Excited to see this issue wrap up and 1.4 RC! Anything we can do to push the last few things along or is it pretty much done waiting to be merged?

[hauleth]

I think that point 1. is already done and seems that rest is covered by #3037 and #3033.