Allow "mix format" to read exported configuration from dependencies #7108

whatyouhide · 2017-12-12T13:24:11Z

~~I am not sure yet on how to go about testing this, any suggestions are appreciated :).~~ Testing is done.

The names of the options have been just decided by me for now so feedback is welcome.

Closes #6644.

ericmj

Do we really need a manifest for this? Is Code.eval_file that much slower than File.read + binary_to_term?

ericmj · 2017-12-12T16:13:37Z

lib/mix/lib/mix/tasks/format.ex

+  # 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(formatter_opts) do
+    case formatter_opts[:import_deps] do


Looks like this should be a cond.

ericmj · 2017-12-12T16:20:59Z

I am not sure yet on how to go about testing this, any suggestions are appreciated

Could you do the same as the other format tests but also write to deps/foo/.formatter.exs?

whatyouhide · 2017-12-13T09:00:54Z

@ericmj that doesn't work because we read dependencies, but I was able to add a test using a fixture project.

ericmj · 2017-12-13T09:02:25Z

lib/mix/test/mix/tasks/format_test.exs

+  test "can read exported configuration from dependencies" do
+    Mix.Project.push(__MODULE__.FormatWithDepsApp)
+
+    in_fixture "format_with_deps", fn ->


If the fixture is empty you can use in_tmp instead.

You're right, I didn't realize we were recreating the dependency directory structure in the test as well. Updated to use in_tmp :)

josevalim · 2017-12-13T10:31:57Z

lib/mix/lib/mix/tasks/format.ex

-      [
-        inputs: ["mix.exs", "{config,lib,test}/**/*.{ex,exs}"]
-      ]
+    * `:input` (a list of paths and patterns) - specifies the default inputs


It should be :inputs.

josevalim · 2017-12-13T10:32:45Z

lib/mix/lib/mix/tasks/format.ex

+       See the "Importing dependencies configuration" section below for more
+       information.
+
+    * `:export_locals_without_parens` (a list of function names and arities) -


I was thinking an :export keys with a :locals_without_parens inside. But this way also works. What do you prefer?

I like them both. Maybe :export with subkeys is more flexible for the future. Shall we go with that?

josevalim · 2017-12-13T10:33:56Z

lib/mix/lib/mix/tasks/format.ex

+        # Regular formatter configuration for my_dep
+        # ...
+
+        export_locals_without_parens: [some_dsl_call: :*]


It is better to give examples without :* so developers don't overuse it.

josevalim · 2017-12-13T10:34:26Z

lib/mix/lib/mix/tasks/format.ex

+  # 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(formatter_opts) do
+    deps = formatter_opts[:import_deps]


Keyword.get(formatter_opts, :import_deps, []) so we can remove the nil check?

I used this initially but removed it because I thought it made sense. Not sure anymore, so I'll just reintroduce this 😄

josevalim · 2017-12-13T10:38:52Z

lib/mix/lib/mix/tasks/format.ex

+  end
+
+  defp deps_manifest() do
+    Path.join(Mix.Project.build_path(), ".cached_deps_formatter.exs")


Mix.Project.manifest_path()

It is also best to remove the .exs extension to avoid issues.

.exs was a residue of initially dumping the Elixir term, good catch. Pushed a fix that uses manifest_path/0 instead of build_path/0 as well.

josevalim · 2017-12-13T10:40:33Z

lib/mix/lib/mix/tasks/format.ex

+    for dep <- deps,
+        dep_dot_formatter = Path.join(Map.fetch!(deps_paths, dep), ".formatter.exs"),
+        File.regular?(dep_dot_formatter),
+        {dep_opts, _} = Code.eval_file(dep_dot_formatter),


What if dep_opts is not a keyword list?

Pushed a fix that extracts reading a kw list from a file (using this when reading .formatter.exs as well).

josevalim · 2017-12-13T10:41:28Z

lib/mix/lib/mix/tasks/format.ex

+            dep_parenless_calls = eval_deps_opts(deps)
+
+            if dep_parenless_calls != [] do
+              write_deps_manifest(dep_parenless_calls)


If we don't write the file, the manifest is always out of date and we will execute this every time. It is probably best to write the file even if it will be an empty list.

That makes sense, but do we want to write it even if folks don't provide :export_locals_without_parens? That might be why I wasn't using Keyword.get(..., []).

This is the case where the user specifies some deps but they don't export anything.

josevalim · 2017-12-13T10:57:29Z

lib/mix/lib/mix/tasks/format.ex

+    deps_paths = Mix.Project.deps_paths()
+
+    for dep <- deps,
+        dep_dot_formatter = Path.join(Map.fetch!(deps_paths, dep), ".formatter.exs"),


What happens if an invalid dep is given? Should we have a better error message?

Pushed a fix that validates dependencies with a nice error message if something goes wrong.

josevalim · 2017-12-13T11:55:52Z

lib/mix/lib/mix/tasks/format.ex

+    deps_paths = Mix.Project.deps_paths()
+
+    for dep <- deps,
+        ensure_valid_dependency!(dep, deps_paths),


path = fetch_dependency_path!(dep, deps_paths)?

josevalim · 2017-12-13T11:56:51Z

lib/mix/test/mix/tasks/format_test.exs

+      File.touch!(manifest_path, {{1970, 1, 1}, {0, 0, 0}})
+      %File.Stat{mtime: mtime} = File.stat!(manifest_path)
+
+      File.touch!(".formatter.exs")


I don't think this is necessary?

josevalim · 2017-12-13T11:57:03Z

lib/mix/test/mix/tasks/format_test.exs

+
+      # Let's check that the manifest gets updated if .formatter.exs changes.
+      File.touch!(manifest_path, {{1970, 1, 1}, {0, 0, 0}})
+      %File.Stat{mtime: mtime} = File.stat!(manifest_path)


We know the mtime, there is no reason to read it back!

josevalim · 2017-12-13T11:57:30Z

lib/mix/test/mix/tasks/format_test.exs

+      Mix.Tasks.Format.run(["a.ex"])
+      %File.Stat{mtime: new_mtime} = File.stat!(manifest_path)
+
+      assert new_mtime > mtime


Other places in the suite we would do:

assert File.stat!(manifest_path).mtime > {{1970, 1, 1}, {0, 0, 0}}

josevalim · 2017-12-13T13:42:36Z

❤️ 💚 💙 💛 💜

whatyouhide · 2017-12-13T13:43:05Z

🎉

wojtekmach · 2017-12-13T16:55:35Z

lib/mix/lib/mix/tasks/format.ex

+
+  This task supports importing formatter configuration from dependencies.
+
+  A dependency that wants to export formatter configuration needs to have a `.formatter.exs` file


strictly speaking, for Hex dependencies the .formatter.exs file needs to be in the package and by default it won't be [1]. I think we could add it by default, or make a note here, or hope maintainers will remember to set it. WDYT?

[1] https://github.com/hexpm/hex/blob/v0.17.1/lib/mix/tasks/hex.build.ex#L4

@wojtekmach let's update hex?

whatyouhide requested a review from josevalim December 12, 2017 13:24

ericmj reviewed Dec 12, 2017

View reviewed changes

ericmj reviewed Dec 13, 2017

View reviewed changes

whatyouhide changed the title ~~[WIP] Allow "mix format" to read exported configuration from dependencies~~ Allow "mix format" to read exported configuration from dependencies Dec 13, 2017

josevalim reviewed Dec 13, 2017

View reviewed changes

whatyouhide force-pushed the al/formatter-deps branch from 66b55f5 to 5b9c8fb Compare December 13, 2017 12:07

whatyouhide added 10 commits December 13, 2017 13:40

Allow "mix format" to read exported configuration from dependencies

fe6ffd6

case -> cond

ae2ae20

Add a test

1766a3c

Fix a logic error

9b2ef68

Implement Eric's feedback

ff7aecb

Fix minor things

5faa656

Fix a bunch of things

b1e0f6a

Update docs

cd1b39a

Implement José's feedback

6cc113a

Improve manifest name

cd95dcc

whatyouhide force-pushed the al/formatter-deps branch from 5b9c8fb to cd95dcc Compare December 13, 2017 12:40

josevalim merged commit 91ededc into master Dec 13, 2017

josevalim deleted the al/formatter-deps branch December 13, 2017 13:42

wojtekmach reviewed Dec 13, 2017

View reviewed changes


		This task supports importing formatter configuration from dependencies.

		A dependency that wants to export formatter configuration needs to have a `.formatter.exs` file

Allow "mix format" to read exported configuration from dependencies #7108

Allow "mix format" to read exported configuration from dependencies #7108

Uh oh!

Conversation

whatyouhide commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericmj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericmj commented Dec 12, 2017

Uh oh!

whatyouhide commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim commented Dec 13, 2017

Uh oh!

whatyouhide commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

whatyouhide commented Dec 12, 2017 •

edited

Loading

josevalim Dec 13, 2017 •

edited

Loading