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 mix-format for .heex files #304

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

tomconroy
Copy link

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".

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
Copy link
Author

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+)

raxod502
raxod502 previously approved these changes May 17, 2024
Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ Ok, LGTM, I am a bit worried that Ubuntu 23+ is a pretty high minimum version requirement, but it is easy to customize the command back to the old version if necessary, and if a lot of people run into difficulty out of the box then we could add a wrapper script to intelligently pick the correct command-line based on the version.

@raxod502
Copy link
Member

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?

@tomconroy
Copy link
Author

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

@raxod502
Copy link
Member

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

@tomconroy
Copy link
Author

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?

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.

2 participants