lassik / emacs-format-all-the-code

Auto-format source code in many languages with one command
https://melpa.org/#/format-all
MIT License
605 stars 105 forks source link

Elixir mix format ignores .formatter.exs because its not run in the project directory #36

Closed strayer closed 5 years ago

strayer commented 5 years ago

mix format expects the .formatter.exs in the current working directory: https://hexdocs.pm/mix/master/Mix.Tasks.Format.html

In a usual Elixir project, mix format could be run from the project directory or explicitly specify the .formatter.exs when formatting a buffer.

lassik commented 5 years ago

Thanks for reporting this. I don't use Elixir myself so I wasn't aware it doesn't work intuitively.

So whether we choose to run the formatter from the project root directory, or give it the .formatter.exs pathname, in either case we have to find the project root. Does mix have an easy way to find it for us, or do we need to write our own root-finding function in Emacs Lisp?

lassik commented 5 years ago

Something like "--dot-formatter" (format-all-find-file-in-directory-or-parents ".formatter.exs")

strayer commented 5 years ago

I don't think the mix cli can help in this situation. I tried various things, but as far as I can see, it is expected to always run mix in the project root (otherwise it often complains because it can't find its mix.exs project file)

The usual situation would be that the user edits an Elixir file that is part of an Elixir mix project. I think the smartest way to find the .formatter.exs in this case would be to recursively find the parent directory that contains the mix.exs (essentially something like a package.json for Node projects) and use the .formatter.exs next to it as --dot-formatter, if it exists. Just going upwards and selecting the first .formatter.exs that can be found is not really the same, although I assume it would work in most situations. As far as I know, most other software runs mix with the mix project root as its working directory.

Elixir files can exist without a mix project, so when no parent mix.exs can be found, the --dot-formatter parameter should simply be left out to let mix format do its own thing, I guess.

I think the user should be able to override this bevahiour, but as far as I understand, there are already several discussions about the topic of customizing/overriding behaviour of this project, so I won't go into it here too :)

strayer commented 5 years ago

This may be relatively easy with locate-dominating-file (see StackExchange):

(locate-dominating-file FILE NAME)

Look up the directory hierarchy from FILE for a directory containing NAME. Stop at the first parent directory containing a file NAME, and return the directory. Return nil if not found. Instead of a string, NAME can also be a predicate taking one argument (a directory) and returning a non-nil value if that directory is the one for which we're looking.

I'm not very good with elisp though, so I'm not too sure I can build something that looks for a mix.exs and returns the .formatter.exs path, if it exists in the same directory.

strayer commented 5 years ago

I came up with this, although I can't say if this is good code:

(defun find-elixir-formatter-exs ()
  "Tries to find the .formatter.exs of an Elixir mix project"
  (interactive)
  (when (locate-dominating-file default-directory "mix.exs")
    (when (file-exists-p (concat (file-name-as-directory (locate-dominating-file default-directory "mix.exs")) ".formatter.exs"))
      (message (file-relative-name (concat (file-name-as-directory (locate-dominating-file default-directory "mix.exs")) ".formatter.exs")))
    )
  )
)

This is an interactive command that outputs the path of the .formatter.exs of the Elixir project root, if it can find one. If you can give me some directions on how I could integrate this in emacs-format-all-the-code, I'd be happy to try my first real elisp pull request! I guess it could go like this:

  1. add a defun format-all-find-elixir-formatter-exs (see function above)
  2. add when condition to :format of mix-format to add --dot-formatter, like in rufo and others
lassik commented 5 years ago

Thanks for working hard on this!

You found just the right functions to use: locate-dominating-file and file-relative-name. I had forgotten all about them.

Your function can be written in idiomatic Elisp as follows:

(defun find-elixir-formatter-exs ()
  "Tries to find the .formatter.exs of an Elixir mix project"
  (interactive)
  (let ((project-root (locate-dominating-file default-directory "mix.exs")))
    (when project-root
      (let ((formatter-file (concat project-root ".formatter.exs")))
        (when (file-exists-p formatter-file)
          (message "%S" (file-relative-name formatter-file)))))))

The pathname returned by locate-dominating-file is already in the form given by file-name-as-directory (Elisp functions usually return directory names with the final slash so you can just use ordinary string concatenation to append filenames to them, i.e. concat. I don't know whether there's a separate path-join function.)

Local variables in Lisp are made with let which is counter-intuitive to people coming from most languages, but since you work within the Erlang ecosystem it's probably not as weird to you :)

The first argument of message is a format string (like printf() in many languages), hence the "%S".

Lisp programmers put all closing parentheses on the same line and let the editor count them (when you type a closing paren, Emacs blinks the matching opening paren so you know when to stop typing more parentheses :) M-x indent-region fixes any wrong indentation so you get an accurate idea of the structure of the code from the indentation. We rarely count parentheses since the editor does all the work for us.

lassik commented 5 years ago

We could generalize this whole mechanism to other formatters since elm-format has a similar issue (i.e. recent versions have to be run from the project root directory or it cannot find its configuration files). Something like changing

(define-format-all-formatter mix-format
  (:executable "mix")
  (:install (macos "brew install elixir"))
  (:modes elixir-mode)
  (:format (format-all-buffer-easy executable "format" "-")))

to add a new keyword like:

  (:project-root "mix.exs")

So the formatter would always be run from the directory with mix.exs if such a file can be found.

Does mix format need the --dot-formatter argument even when it's run from the mix.exs directory, or does it find .formatter.exs automatically in that case?

strayer commented 5 years ago

Does mix format need the --dot-formatter argument even when it's run from the mix.exs directory, or does it find .formatter.exs automatically in that case?

It would work just fine in that case, so the generic :project-root option would probably make sense, yes.

lassik commented 5 years ago

OK, let's do that :) I'll implement it and fix elm-format at the same time.

lassik commented 5 years ago

Just to confirm, does mix format work fine if you have a mix.exs but no .formatter.exs? I.e. does it gracefully fall back to default settings in that case

strayer commented 5 years ago

Yes, that is essentially what it is doing right now when the buffer is formatted. The mix.exs is irrelevant for mix format, as far as I know. I‘m away for the weekend, but can verify this on Monday.

strayer commented 5 years ago

I just verified this. When there is no .formatter.exs, mix format will fallback to defaults, regardless of where it is run – as long as the correct relative path to the file to be formatted is given.

lassik commented 5 years ago

@Strayer Sorry for taking forever to get to this. I didn't realize it's been almost a month already.

In the end I decided against adding (:project-root "mix.exs") to the formatter definitions, and added an extra argument to the format-all-buffer-hard function instead. This feature seems a bit specialized and slightly hacky so I thought it would be better to just keep it in a low-key utility function.

Anyway, mix-format should work with .formatter.exs files now. Can you confirm?

lassik commented 5 years ago

Also fixed elm-format while at it.

lassik commented 5 years ago

Also, if you do (setq format-all-debug t) the *Messages* buffer now gets Format-All: Directory: <...> lines that say which working directory is used for each formatter.

strayer commented 5 years ago

Mhh, this doesn't seem to work, sadly. I updated all packages and tried formatting a file in a subdirectory, but it seems mix format is still run in the files directory:

Format-All: Running: /Users/work/.asdf/shims/mix format -
Format-All: Directory: /Users/work/dev/redacted/redacted-backend/lib/redacted_web/
Formatted (mix-format)
~/dev/redacted/redacted-backend/lib/redacted_web
❯ file mix.exs
mix.exs: cannot open `mix.exs' (No such file or directory)

~/dev/redacted/redacted-backend/lib/redacted_web
❯ file ../mix.exs
../mix.exs: cannot open `../mix.exs' (No such file or directory)

~/dev/redacted/redacted-backend/lib/redacted_web
❯ file ../../mix.exs
../../mix.exs: Ruby script text, ASCII text # (no idea why file thinks this is a ruby script)
lassik commented 5 years ago

Interesting. I tried making a mix.exs file in the parent directory of an example Elixir file and it works on my computer. Is there a public repo that I can clone to try and reproduce the problem?

strayer commented 5 years ago

I'm now getting this error, I assume something went wrong todays commits: let*: Autoloading file /Users/strayer/.emacs.d/.local/packages/elpa/format-all-20190606.853/format-all.elc failed to define function format-all-probe

I still created an example project (it really is just mix phx.new format_all_example) at https://github.com/Strayer/emacs-format-all-the-code-issue-36

You can try the formatter in lib/format_all_example_web/router.ex – it will add various braces that are not required to be there based on the .formatter.exs.

I'm using Elixir 1.8.2 with Erlang 22.0.2 on macOS mojave with doom-emacs.

lassik commented 5 years ago

Thank you very much for taking the time to make an example repo.

I tried it on the file you suggest and it works fine for me. The formatter does not insert any extra braces.

I'm using Elixir 1.8.2 with Erlang 22.0.2 on macOS mojave with doom-emacs.

Doom may explain it. They add so many modifications to the base version of format-all that I have no idea what my own package does anymore. Then people send strange bug reports to format-all from time to time and we are all confused :)

I'm now getting this error, I assume something went wrong todays commits: let*: Autoloading file /Users/strayer/.emacs.d/.local/packages/elpa/format-all-20190606.853/format-all.elc failed to define function format-all-probe

This is probably caused by today's commit https://github.com/lassik/emacs-format-all-the-code/commit/f3ed09c430c80d17fcc34f130f32072177f4ef2b in which private functions were changed to use the naming convention suggested in the Emacs Lisp manual. I think Doom uses the private functions directly so the change messed up some of their hacks.

strayer commented 5 years ago

Oh, I didn't realize that! Sorry for causing confusion and wasting your time ☹️

I'll create an issue in the doom issue tracker for both of these problems and see how it will go from there.

lassik commented 5 years ago

No problem :) It's not your fault at all, you did not cause the confusion. Thanks a lot for taking it up with them.