Allow subdirectories in .formatter.exs #7398
Conversation
josevalim
force-pushed
the
jv-formatter-subdir
branch
from
101a784 to
d9f74b1
Compare
There was a problem hiding this comment.
Overall it looks good. As far as I can tell, we don't merge the option in a/b/.formatter.exs with the options in a/.formatter.exs when formatting a/b/foo.ex, right? If I got it correctly and it is like this, what is the reason why instead of doing the whole dance of finding the "closest" formatter file we don't just run mix format from the directory where each .formatter.exs is in? We could still cache the location and contents of each .formatter.exs I think. I am sure I am missing something but was worth asking :)
| end | ||
|
|
||
| defp read_deps_manifest(deps_manifest) do | ||
| deps_manifest |> File.read!() |> :erlang.binary_to_term() | ||
| defp write_manifest!(manifest, {entry, sources}) do |
There was a problem hiding this comment.
This might be nitpicking, but I think it would make more sense from a code clarity point of view if write_manifest!/2 would ignore what's being written. This would mean writing a manifest that looks like:
{@manifest_vsn, term}which I think makes more sense as the shape will stay the same even if we change the manifest version. It would simply return the term being written so this change wouldn't affect anything else in this PR I believe.
There was a problem hiding this comment.
@whatyouhide the reason why I decided to not do so is because the manifest is now responsible to tell if it is out of date or not. We do so to avoid parsing the subdirectories in every command. So the manifest needs to know about the sources and I pushed the versioning concerns into it.
The entry part is completely opaque though.
| if List.starts_with?(split, Path.split(sub)) do | ||
| find_formatter_opts_for_file(split, formatter_opts_and_subs) | ||
| end | ||
| end) |
There was a problem hiding this comment.
Can you elaborate on the reasoning behind doing this? I am having a bit of a hard time understanding it.
There was a problem hiding this comment.
This is the finding the closest .formatter.exs bit for when you invoke the formatter as mix format path/to/some/file.ex.
You are correct. The only reason we don't do so in the directory each .formatter.exs is in is because of the cache. For example, Phoenix plans to put a .formatter.exs inside priv/repo, and simply running in that directory means we have no sensible place to put the cache in. We could devise a schema so we can still cache the inner .formatter.exs to complex paths in the manifest path but I thought keeping everything together is the simplest approach, especially because we still want to support loading dependencies from inner .formatter.exs files and things like |
| @@ -22,6 +22,10 @@ defmodule Mix.Tasks.Format do | |||
| * `:inputs` (a list of paths and patterns) - specifies the default inputs | |||
| to be used by this task. For example, `["mix.exs", "{config,lib,test}/**/*.{ex,exs}"]`. | |||
|
|
|||
| * `:subdirectories` (a list of paths and patterns) - specifies subdirectories | |||
There was a problem hiding this comment.
Overall it looks good. As far as I can tell, we don't merge the option in
a/b/.formatter.exswith the options ina/.formatter.exswhen formattinga/b/foo.ex, right? If I got it correctly and it is like this...
I also had the same doubts, and I had to look at the source code. I think a more detailed explanation about the inheritance rules between .formatter.exs and its :subdirectories could be helpful :)
There was a problem hiding this comment.
I have clarified it here, thank you!
I think this PR could be relevant #7328 :) |
josevalim
force-pushed
the
jv-formatter-subdir
branch
from
d9f74b1 to
d655704
Compare
There was a problem hiding this comment.
@josevalim with the updates to the documentation this PR is much clearer for me, so big 👍 on those. I pushed a commit that slightly rewords that documentation in a way that I think is simpler and more direct but feel free to further reword and merge at will :)
|
@whatyouhide thanks for the changes. You quoted one "lib/app" but not the other, so I went ahead and remove the quotes you added. We can change it later but i don't think it matters. :) |
|
❤️ 💚 💙 💛 💜 |
lib/mix/lib/mix/tasks/format.ex
Outdated
| @@ -26,7 +26,7 @@ defmodule Mix.Tasks.Format do | |||
| that have their own formatting rules. Each subdirectory should have a | |||
| `.formatter.exs` that configures how entries in that subdirectory should be | |||
| formatted as. Configuration between `.formatter.exs` are not shared nor | |||
| inherited. If a `.formatter.exs` lists `"lib/app"` as a subdirectory, the rules | |||
| inherited. If a `.formatter.exs` lists "lib/app" as a subdirectory, the rules | |||
|
@josevalim ah I see now, I didn't notice I missed one. It's fine for now yeah, no problem :) Great job! |
Signed-off-by: José Valim <jose.valim@plataformatec.com.br>
- Set the `default-directory` to where mix.exs is located, because mix is meant to be run from the project root directory. Also, mix format looks for .formatter.exs files in subdirectories. - If mix.exs is NOT found, the original `default-directory` and default formatter are used for mix format. - Create the temp file in the current directory, because mix format uses path patterns. See: - elixir-lang/elixir#7328 (comment) - elixir-lang/elixir#7398 (comment)
No description provided.