Skip to content
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

Fix - Allow to use a dot_formatter config file in a different folder which imports dependencies #7328

Conversation

sascha-wolf
Copy link
Contributor

This PR changes the format.ex mix task and updates the corresponding test.

eval_dot_formatter/1 now returns a two value tuple with the first value being the directory of the evaluated dot_formatter file. The second value are the actual dot_formatter options.

It then passes this formatter_dir to fetch_deps_opts which now has an arity two implementation. This implementation receives the formatter_opts and the formatter_dir as second argument. Take a look at the header.

defp fetch_deps_opts(formatter_opts, in_directory: nil)

If the directory is non nil, it wraps the call to fetch_deps_opts/1 in a File.cd!/2 call. This ensures that the dependencies are actually loaded from the same directory in which the dot_formatter resides.

Fixes #7327

@sascha-wolf sascha-wolf force-pushed the bug/mix-format-with-config-in-different-folder branch 2 times, most recently from fae9f88 to 65167ee Compare February 8, 2018 15:51
Sascha Wolf added 3 commits February 8, 2018 16:53
…lder

The new test case builds on the "can read exported configuration from
dependencies"-test. It tests that `mix format` can be called with
`--dot-formatter ../.formatter.exs` when the `../.formatter.exs`
contains `import_deps: [:my_dep]`.

The `my_dep` dependency in turn can be found in `../deps/my_dep`.

Right now this test fails. It seems like `mix format` tries to load the
dependency from the workdir instead rather than from the same folder the
provided `.formatter.exs` resides.

TL;DR: mix format tries to load dependencies from the wrong folder, when
       passing a `.formatter.exs` in another folder
…ctory

This change fetches the folder from the provided dot_formatter config
file, or it returns nil.

Furthermore it adds a `fetch_deps_opts(formatter_opts, in_directory: formatter_dir)`
function. This function then uses a non nil `formatter_dir` to wrap the
actual `fetch_deps_opts/1` call in a `File.cd!` call for the provided
`formatter_dir`.

Fixes elixir-lang#7327
@sascha-wolf sascha-wolf force-pushed the bug/mix-format-with-config-in-different-folder branch from 65167ee to c077733 Compare February 8, 2018 15:54
@sascha-wolf sascha-wolf force-pushed the bug/mix-format-with-config-in-different-folder branch from fd82ecd to 80c905f Compare February 8, 2018 16:13
@josevalim
Copy link
Member

I don't think this is going to work. Mix was designed to always run from the root. We shouldn't cd into the current directory because even your mix.exs may assume that it is running at the root directory.

In other words, this is not a formatter bug, but it is a consequence that you are not running in the same directory as the mix.exs file and using Mix specific features (such as dependency handling).

I will see if we can improve the error message though.

@sascha-wolf
Copy link
Contributor Author

The only logic which is executed in the dot_formatter folder is loading the dependency configuration for mix format. Nothing more, nothing less. It's not like this changes the behaviour of mix in general.

@josevalim
Copy link
Member

josevalim commented Feb 8, 2018

@zeeker i have tried your test locally and i got this error:

** (Mix.Error) Unavailable dependency :my_dep given to :import_deps in the formatter configuration. The dependency cannot be found in the filesystem, please run mix deps.get and try again

It looks correct to me.

@josevalim
Copy link
Member

Nothing more, nothing less. It's not like this changes the behaviour of mix in general.

It changes, because it breaks the assumption Mix is always running at the root of the project. Once you File.cd! only in certain situations, it will lead to more failures somewhere down the road.

@sascha-wolf
Copy link
Contributor Author

sascha-wolf commented Feb 8, 2018

I can see your point but as a user I was never aware of the fact that mix is meant to be run in the root folder.

On the other hand format does provide the option to use a dot format file from another folder , and it is possible to specify configuration which references data in this folder .

What do we make of this directory bound configuration, if not load it?

@josevalim
Copy link
Member

Well, you can move around freely. But if you are using dependencies, then you need to be in the directory that defines the mix.exs with those dependencies, otherwise we indeed won't find them.

I don't think there is much for us to do here, so I will go ahead and make sure this is clear in the docs, thanks!

@josevalim josevalim closed this Feb 8, 2018
@sascha-wolf
Copy link
Contributor Author

What do you think of another option to either specify the dependency path or to enable the behaviour from this PR?

michaelvobrien added a commit to michaelvobrien/emacs-elixir that referenced this pull request Jun 5, 2018
- 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants