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

Format: .formatter.exs with import_deps results in errors #415

Open
sascha-wolf opened this issue May 28, 2018 · 9 comments
Open

Format: .formatter.exs with import_deps results in errors #415

sascha-wolf opened this issue May 28, 2018 · 9 comments

Comments

@sascha-wolf
Copy link

sascha-wolf commented May 28, 2018

This issue is closely related to elixir-lang/elixir#7327.

Current behaviour

Let's assume I'm in a project with a .formatter.exs file looking like this:

[
  import_deps: [:some_dependency_with_formatting_rules],
  inputs: [
    # Some files here ...
  ],
  locals_without_parens: [
    # Some configurations ...
  ]
]

Now I'm working on a file in lib/my_cool_module.ex and then execute elixir-format. This results in the following error:

** (Mix) Unknown dependency :some_dependency_with_formatting_rules given to :import_deps in the formatter configuration. The dependency is not listed in your mix.exs for environment :dev

The dependency is available in dev and included in the root mix.exs. The issue stems from the fact that mix format tries find the dependencies in the current folder - which is the buffer's default-directory - instead of looking for them in the .formatter.exs folder. Due to that it's unable to find the dependencies and raises this error.

Possible fix

Due to mix format looking for a .formatter.exs file in the current folder by default, it might make sense to allow the user to control the execution directory for elixir-format, this would also make a --dot-formatter configuration obsolete.

In addition to that a sensible default for this folder could be the projectile root folder, as this usually contains the .formatter.exs.

Alternative fix

This could be fixed on the mix level.

I've actually submitted elixir-lang/elixir#7328 a while ago, which would have fixed this issue. Sadly it got rejected due to having to cd into the .formatter.exs folder when loading the dependencies.

Instead it was decided that mix format should raise a more "descriptive" error, which you can see above.

Workaround

As a workaround I point my elixir-format-elixir-path to a bash script with the following content:

if git rev-parse --is-inside-work-tree 1>/dev/null 2>/dev/null; then
  cd "$(git rev-parse --absolute-git-dir | xargs dirname)"
fi

/path/to/my/elixir $@

This script checks if I'm in a git repository and then cds into the root folder of the repository. It basically is a hacked version of the suggested fix above.

@smaximov
Copy link

smaximov commented May 29, 2018

Another workaround is to implement your suggested fix using advices:

(defun set-default-directory-to-mix-project-root (original-fun &rest args)
  (if-let* ((mix-project-root (and (projectile-project-p)
                                   (projectile-locate-dominating-file buffer-file-name
                                                                      ".formatter.exs"))))
      (let ((default-directory mix-project-root))
        (apply original-fun args))
    (apply original-fun args)))

(advice-add 'elixir-format :around #'set-default-directory-to-mix-project-root)

It doesn't requies a wrapper script and seems to work with umbrella projects too.

@Trevoke
Copy link
Contributor

Trevoke commented Jun 5, 2018

@michaelvobrien opened a PR to the repo where the format code came from ( michaelvobrien/mix-format.el@ecde90b ) which goes in this direction too.

It seems like we may need to run through a few potential loops:

  1. Go up until we find a .formatter.exs file, run from that directory if found
  2. Go up until we find a mix.exs file, run from that directory if found
  3. Just format

What do you all think?

@smaximov
Copy link

smaximov commented Jun 5, 2018

I like the idea. I'm not sure if step 1 is necessary though. Is it possible that a .formatter.exs file for a mix project is located not in the directory which also contains the mix.exs file for the project?

@nicocharlery
Copy link
Contributor

@smaximov it is possible yes.
An umbrella project with .formatter.exs defined at the project directory, but with another mix.exs defined an the app. Whether that's correct is another question!

The formatter could then set an import_deps from a dependency that is only defined in the mix.exs that is in the app, leading to that current issue.

# Case 1 
/apps/my_app/.mix
/apps/my_app/.formatter.exs

# Case 2
/apps/my_app/.mix.exs
/.formatter.exs

# Case 3
/.mix.exs
/.formatter.exs

Note from https://hexdocs.pm/mix/Mix.Tasks.Format.html#module-formatting-options:

: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 the mix.exs file that defines those dependencies.

So I think that in @Trevoke proposal, step 1 and 2 should be swapped (and possibly merged), but when running from the dominating mix.exs, we should also look for the first dominating .formatter.exs as well.

@sascha-wolf
Copy link
Author

Usually in umbrella applications the sub-apps point to the _build folder in the project root. In fact they get created that way by mix.

So executing mix format still would happen in the directory root.

@smaximov
Copy link

smaximov commented Jun 9, 2018

So executing mix format still would happen in the directory root.

I believe it should work only if the umbrella root has a .formatter.exs file with :subdirectories pointing to the umbrella sub-applications (e.g. [subdirectories: ["apps/*"]]), which totally makes sense for umbrella projects, though it's not created by default (I missed this setting and found out about it only after I re-read the docs after reading your comment).

@smaximov
Copy link

smaximov commented Jun 9, 2018

though it's not created by default

Disregard that, mix new generates correct .formatter.exs files for regular and umbrella projects. It's the Phoenix generator that doesn't generate .formatter.exs at the moment.

@victorolinasc
Copy link
Contributor

Just to add to the discussion...

Another way to detect if we are in an umbrella is to run what exunit.el does (this is very elegant IMHO):

Code by @ananthakumaran

(defvar-local exunit-project-root nil)
(make-variable-buffer-local 'exunit-project-root)

(defvar-local exunit-umbrella-project-root nil)
(make-variable-buffer-local 'exunit-umbrella-project-root)

(defun exunit-project-root ()
  "Return the current project root.
This value is cached in a buffer local to avoid filesytem access
on every call."
  (or
   exunit-project-root
   (let ((root (locate-dominating-file default-directory "mix.exs")))
     (unless root
       (error "Couldn't locate project root folder.  Make sure the current file is inside a project"))
     (setq exunit-project-root (expand-file-name root)))))

(defun exunit-umbrella-project-root ()
  "Return the current umbrella root.
This value is cached in a buffer local to avoid filesytem access
on every call."
  (or
   exunit-umbrella-project-root
   (let ((root (locate-dominating-file default-directory "apps")))
     (unless root
       (error "Couldn't locate umbrella root folder.  Make sure the current file is inside a umbrella project"))
(setq exunit-umbrella-project-root (expand-file-name root)))))

Of course that to use mix format properly we would need to "try" finding "apps" first and then, if not found, we would fall back to current project root.

@emmy0x1
Copy link

emmy0x1 commented Apr 12, 2020

@sascha-wolf

How did you add this bash script. Is elixir-format-elixir-path something you put in config.el? Can't seem to find docs for this.

I got no match doing this:

M-x customize-option

Customize-variable: elixir-format-mix-path

J3RN pushed a commit to J3RN/emacs-elixir that referenced this issue Apr 24, 2021
* move to provider

* no need to feature check as formatter and references are available since elixir 1.7 and we require 1.8

* elixir_sense suggestions for erlang modules now properly include :

no need to patch

* elixir_sense definitions now return nil when not found

extract location  related code to a new module

* add implementations provider

* update elixir_sense

* do not format test/tmp fixtures

* move fixtures to common dir

* do not build filesystem URIs by string concat as it will break on Windows

* add tests

* fix invalid uris

* Revert "do not format test/tmp fixtures"

This reverts commit 5012101bc4ba31052d26fbb4e184a624a75a6c76.

* Revert "fix invalid uris"

This reverts commit 38eeb67c129384aa4343e5a546d7a7c0fe159779.

* run formatter

* increase timeout

* bump elixir_sense

* don't catch everyting

* bump elixir_sense

fix tests on elixir < 1.11
J3RN pushed a commit to J3RN/emacs-elixir that referenced this issue Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants