apheleia icon indicating copy to clipboard operation
apheleia copied to clipboard

Fix mix-format for `.heex` files

Open tomconroy opened this issue 1 year ago • 5 comments

Per the documentation for mix format:

  • --stdin-filename - path to the file being formatted on stdin. This is
    useful if you are using plugins to support custom filetypes such as .heex.
    Without passing this flag, it is assumed that the code being passed via
    stdin is valid Elixir code. Defaults to "stdin.exs".

tomconroy avatar May 16 '24 20:05 tomconroy

It seems the test is failing because the test suite is using elixir 1.12 (because of ubuntu 22) and this option was added in 1.14 (available on ubuntu 23+)

tomconroy avatar May 16 '24 20:05 tomconroy

Oh I see, as you said this blocks CI. Hmmm, can we implement the wrapper script now then, so that it works for CI but also for everyone who is not running the latest package version?

raxod502 avatar May 17 '24 22:05 raxod502

I can give it a shot, is there an example in this repo of a similar behavior?

tomconroy avatar May 20 '24 14:05 tomconroy

There aren't any other formatters that have conditioning on the version in use currently, but there are other wrapper scripts in the scripts/formatters directory. I would do something like running the tool to get its version, cache that in a file with some ttl or based on some other property of the system like the executable location, then delegate the correct arguments based on that. Should be fast and robust.

If you just want to keep things simply for now, I am okay with that too, like I mentioned we can change it later if we receive complaints. Looks like the formatting sample data needs to be updated though to reflect the new tool version:

image

raxod502 avatar May 23 '24 22:05 raxod502

So I tried adding a new test for .heex formatting but I realized we would need a config file (.formatter.exs) and probably some elixir dependencies to get it work (https://hexdocs.pm/phoenix_live_view/Phoenix.LiveView.HTMLFormatter.html) Maybe I should just remove this new test?

tomconroy avatar May 28 '24 14:05 tomconroy