Skip to content

Allow subdirectories in .formatter.exs #7398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 28, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
210 changes: 148 additions & 62 deletions lib/mix/lib/mix/tasks/format.ex
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@ 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have clarified it here, thank you!

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
in `.formatter.exs` won't be available in `lib/app/.formatter.exs`.
Note that the parent `.formatter.exs` must not specify files inside the "lib/app"
subdirectory in its `:inputs` configuration. If this happens, the behaviour of
which formatter configuration will be picked is unspecified.

* `:import_deps` (a list of dependencies as atoms) - specifies a list
of dependencies whose formatter configuration will be imported.
When specified, the formatter should run in the same directory as
Expand Down Expand Up @@ -115,16 +125,19 @@ defmodule Mix.Tasks.Format do
dry_run: :boolean
]

@deps_manifest "cached_formatter_deps"
@manifest "cached_dot_formatter"
@manifest_vsn 1

def run(args) do
{opts, args} = OptionParser.parse!(args, strict: @switches)
{dot_formatter, formatter_opts} = eval_dot_formatter(opts)
formatter_opts = fetch_deps_opts(dot_formatter, formatter_opts)

{formatter_opts_and_subs, _sources} =
eval_deps_and_subdirectories(dot_formatter, [], formatter_opts, [dot_formatter])

args
|> expand_args(formatter_opts)
|> Task.async_stream(&format_file(&1, opts, formatter_opts), ordered: false, timeout: 30000)
|> expand_args(dot_formatter, formatter_opts_and_subs)
|> Task.async_stream(&format_file(&1, opts), ordered: false, timeout: 30000)
|> Enum.reduce({[], [], []}, &collect_status/2)
|> check!()
end
Expand All @@ -142,71 +155,110 @@ defmodule Mix.Tasks.Format do
end
end

# This function reads exported configuration from the imported dependencies and deals with
# caching the result of reading such configuration in a manifest file.
defp fetch_deps_opts(dot_formatter, formatter_opts) do
# This function reads exported configuration from the imported
# dependencies and subdirectories and deals with caching the result
# of reading such configuration in a manifest file.
defp eval_deps_and_subdirectories(dot_formatter, prefix, formatter_opts, sources) do
deps = Keyword.get(formatter_opts, :import_deps, [])
subs = Keyword.get(formatter_opts, :subdirectories, [])

cond do
deps == [] ->
formatter_opts

is_list(deps) ->
# Since we have dependencies listed, we write the manifest even if those
# dependencies don't export anything so that we avoid lookups everytime.
deps_manifest = Path.join(Mix.Project.manifest_path(), @deps_manifest)
dep_parenless_calls = maybe_cache_eval_deps_opts(dot_formatter, deps_manifest, deps)

Keyword.update(
formatter_opts,
:locals_without_parens,
dep_parenless_calls,
&(&1 ++ dep_parenless_calls)
)
if not is_list(deps) do
Mix.raise("Expected :import_deps to return a list of dependencies, got: #{inspect(deps)}")
end

true ->
Mix.raise("Expected :import_deps to return a list of dependencies, got: #{inspect(deps)}")
if not is_list(subs) do
Mix.raise("Expected :subdirectories to return a list of directories, got: #{inspect(subs)}")
end

if deps == [] and subs == [] do
{{formatter_opts, []}, sources}
else
manifest = Path.join(Mix.Project.manifest_path(), @manifest)

maybe_cache_in_manifest(dot_formatter, manifest, fn ->
{subdirectories, sources} = eval_subs_opts(subs, prefix, sources)
{{eval_deps_opts(formatter_opts, deps), subdirectories}, sources}
end)
end
end

defp maybe_cache_eval_deps_opts(dot_formatter, deps_manifest, deps) do
defp maybe_cache_in_manifest(dot_formatter, manifest, fun) do
cond do
dot_formatter != ".formatter.exs" ->
eval_deps_opts(deps)

deps_dot_formatters_stale?(dot_formatter, deps_manifest) ->
write_deps_manifest(deps_manifest, eval_deps_opts(deps))
is_nil(Mix.Project.get()) or dot_formatter != ".formatter.exs" -> fun.()
entry = read_manifest(manifest) -> entry
true -> write_manifest!(manifest, fun.())
end
end

true ->
read_deps_manifest(deps_manifest)
def read_manifest(manifest) do
with {:ok, binary} <- File.read(manifest),
{:ok, {@manifest_vsn, entry, sources}} <- safe_binary_to_term(binary),
expanded_sources = Enum.flat_map(sources, &Path.wildcard(&1, match_dot: true)),
false <- Mix.Utils.stale?(Mix.Project.config_files() ++ expanded_sources, [manifest]) do
{entry, sources}
else
_ -> nil
end
end

defp deps_dot_formatters_stale?(dot_formatter, deps_manifest) do
Mix.Utils.stale?([dot_formatter | Mix.Project.config_files()], [deps_manifest])
defp safe_binary_to_term(binary) do
{:ok, :erlang.binary_to_term(binary)}
rescue
_ -> :error
end

defp read_deps_manifest(deps_manifest) do
deps_manifest |> File.read!() |> :erlang.binary_to_term()
defp write_manifest!(manifest, {entry, sources}) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

File.mkdir_p!(Path.dirname(manifest))
File.write!(manifest, :erlang.term_to_binary({@manifest_vsn, entry, sources}))
{entry, sources}
end

defp write_deps_manifest(deps_manifest, parenless_calls) do
File.mkdir_p!(Path.dirname(deps_manifest))
File.write!(deps_manifest, :erlang.term_to_binary(parenless_calls))
parenless_calls
defp eval_deps_opts(formatter_opts, []) do
formatter_opts
end

defp eval_deps_opts(deps) do
defp eval_deps_opts(formatter_opts, deps) do
deps_paths = Mix.Project.deps_paths()

for dep <- deps,
dep_path = assert_valid_dep_and_fetch_path(dep, deps_paths),
dep_dot_formatter = Path.join(dep_path, ".formatter.exs"),
File.regular?(dep_dot_formatter),
dep_opts = eval_file_with_keyword_list(dep_dot_formatter),
parenless_call <- dep_opts[:export][:locals_without_parens] || [],
uniq: true,
do: parenless_call
parenless_calls =
for dep <- deps,
dep_path = assert_valid_dep_and_fetch_path(dep, deps_paths),
dep_dot_formatter = Path.join(dep_path, ".formatter.exs"),
File.regular?(dep_dot_formatter),
dep_opts = eval_file_with_keyword_list(dep_dot_formatter),
parenless_call <- dep_opts[:export][:locals_without_parens] || [],
uniq: true,
do: parenless_call

Keyword.update(
formatter_opts,
:locals_without_parens,
parenless_calls,
&(&1 ++ parenless_calls)
)
end

defp eval_subs_opts(subs, prefix, sources) do
{subs, sources} =
Enum.flat_map_reduce(subs, sources, fn sub, sources ->
prefix = Path.join(prefix ++ [sub])
{Path.wildcard(prefix), [Path.join(prefix, ".formatter.exs") | sources]}
end)

Enum.flat_map_reduce(subs, sources, fn sub, sources ->
sub_formatter = Path.join(sub, ".formatter.exs")

if File.exists?(sub_formatter) do
formatter_opts = eval_file_with_keyword_list(sub_formatter)

{formatter_opts_and_subs, sources} =
eval_deps_and_subdirectories(:in_memory, [sub], formatter_opts, sources)

{[{sub, formatter_opts_and_subs}], sources}
else
{[], sources}
end
end)
end

defp assert_valid_dep_and_fetch_path(dep, deps_paths) when is_atom(dep) do
Expand Down Expand Up @@ -243,22 +295,20 @@ defmodule Mix.Tasks.Format do
opts
end

defp expand_args([], formatter_opts) do
if inputs = formatter_opts[:inputs] do
expand_files_and_patterns(List.wrap(inputs), ".formatter.exs")
else
defp expand_args([], dot_formatter, formatter_opts_and_subs) do
if no_entries_in_formatter_opts?(formatter_opts_and_subs) do
Mix.raise(
"Expected one or more files/patterns to be given to mix format " <>
"or for a .formatter.exs to exist with an :inputs key"
"or for a .formatter.exs to exist with an :inputs or :subdirectories key"
)
end
end

defp expand_args(files_and_patterns, _formatter_opts) do
expand_files_and_patterns(files_and_patterns, "command line")
dot_formatter
|> expand_dot_inputs([], formatter_opts_and_subs, %{})
|> Enum.uniq()
end

defp expand_files_and_patterns(files_and_patterns, context) do
defp expand_args(files_and_patterns, _dot_formatter, formatter_opts_and_subs) do
files =
for file_or_pattern <- files_and_patterns,
file <- stdin_or_wildcard(file_or_pattern),
Expand All @@ -267,12 +317,48 @@ defmodule Mix.Tasks.Format do

if files == [] do
Mix.raise(
"Could not find a file to format. The files/patterns from #{context} " <>
"Could not find a file to format. The files/patterns given to command line " <>
"did not point to any existing file. Got: #{inspect(files_and_patterns)}"
)
end

files
for file <- files do
if file == :stdin do
{file, []}
else
split = file |> Path.relative_to_cwd() |> Path.split()
{file, find_formatter_opts_for_file(split, formatter_opts_and_subs)}
end
end
end

defp expand_dot_inputs(dot_formatter, prefix, {formatter_opts, subs}, acc) do
if no_entries_in_formatter_opts?({formatter_opts, subs}) do
Mix.raise("Expected :inputs or :subdirectories key in #{dot_formatter}")
end

map =
for input <- List.wrap(formatter_opts[:inputs]),
file <- Path.wildcard(Path.join(prefix ++ [input])),
do: {file, formatter_opts},
into: %{}

Enum.reduce(subs, Map.merge(acc, map), fn {sub, formatter_opts_and_subs}, acc ->
sub_formatter = Path.join(sub, ".formatter.exs")
expand_dot_inputs(sub_formatter, [sub], formatter_opts_and_subs, acc)
end)
end

defp find_formatter_opts_for_file(split, {formatter_opts, subs}) do
Enum.find_value(subs, formatter_opts, fn {sub, formatter_opts_and_subs} ->
if List.starts_with?(split, Path.split(sub)) do
find_formatter_opts_for_file(split, formatter_opts_and_subs)
end
end)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on the reasoning behind doing this? I am having a bit of a hard time understanding it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the finding the closest .formatter.exs bit for when you invoke the formatter as mix format path/to/some/file.ex.

end

defp no_entries_in_formatter_opts?({formatter_opts, subs}) do
is_nil(formatter_opts[:inputs]) and subs == []
end

defp stdin_or_wildcard("-"), do: [:stdin]
Expand All @@ -286,7 +372,7 @@ defmodule Mix.Tasks.Format do
{File.read!(file), file: file}
end

defp format_file(file, task_opts, formatter_opts) do
defp format_file({file, formatter_opts}, task_opts) do
{input, extra_opts} = read_file(file)
output = IO.iodata_to_binary([Code.format_string!(input, extra_opts ++ formatter_opts), ?\n])

Expand Down
3 changes: 2 additions & 1 deletion lib/mix/lib/mix/tasks/new.ex
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,8 @@ defmodule Mix.Tasks.New do
embed_template(:formatter_umbrella, """
# Used by "mix format"
[
inputs: ["mix.exs", "apps/*/mix.exs", "apps/*/{config,lib,test}/**/*.{ex,exs}"]
inputs: ["mix.exs", "config/*.exs"],
subdirectories: ["apps/*"]
]
""")

Expand Down
Loading