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

Add Elixir language support with exfmt #2

Merged
merged 2 commits into from
Jun 28, 2017

Conversation

kumekay
Copy link
Contributor

@kumekay kumekay commented Jun 26, 2017

No description provided.

README.md Outdated
@@ -31,14 +32,14 @@ See list of beautifier Docker images at https://hub.docker.com/u/unibeautify/
### Using Whalebrew

1. Install [Whalebrew](https://github.com/bfirsh/whalebrew)
2. `whalebrew install unibeautify/BEAUTIFIER`
Copy link
Member

Choose a reason for hiding this comment

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

These spaces are actually intentional 😉 .

@Glavin001
Copy link
Member

Just setup the new Docker repository: https://hub.docker.com/r/unibeautify/exfmt/builds/

Going to make sure it builds again fine (Travis CI says it does) and then we can merge.

Would you be able to undo the Readme changes? I prefer the e.g. ... line on a new line. What are your thoughts?

@kumekay
Copy link
Contributor Author

kumekay commented Jun 27, 2017

You are right, new lines look better

@Glavin001
Copy link
Member

Thank you for contributing!

@Glavin001 Glavin001 merged commit a246991 into Unibeautify:master Jun 28, 2017
@stevenzeck
Copy link
Contributor

@kumekay It looks like with the upcoming release of Elixir 1.6, code formatting will be integrated into the installation of the Elixir language (https://hashrocket.com/blog/posts/format-your-elixir-code-now). I've never used Elixir so I'll rely on those that have, but would it be better to use that versus exfmt? That's the way Go files are formatted with Atom Beautify, it uses the built-in gofmt command. That way it won't require a separate Docker image, it would be built into the base Elixir image when it's released. Right now exfmt requires Elixir 1.6-dev anyways, which isn't even available as a Docker image and thus is failing.

@Glavin001
Copy link
Member

I've never used Elixir so I'll rely on those that have, but would it be better to use that versus exfmt?

Atom-Beautify should support both formatters, extfmt and built-in. Users can decide which to use, if not both -- not sure if only certain features are supported on one or the other 😉.

@kumekay
Copy link
Contributor Author

kumekay commented Oct 30, 2017

As exfmt author metioned in lpil/exfmt#86

When code_formatter is released this project will be redundant. Until then 🤷

So yes, I think it should support only built in one

@Glavin001
Copy link
Member

Thanks for the info @kumekay .

Would users of Elixir version <= 1.5 be able to use this new builtin formatter? I would assume not, therefore we should make the builtin formatter default, and extfmt a fallback for legacy users.

@stevenzeck
Copy link
Contributor

stevenzeck commented Oct 30, 2017 via email

@Glavin001
Copy link
Member

extfmt requires 1.6 though

Sounds like there is absolutely no reason for extfmt then.

Pull Requests welcome to add builtin formatter support and remove old exfmt!

@stevenzeck
Copy link
Contributor

Yea, once 1.6 is official I'll submit a PR in Atom Beautify to change the executable. The Docker image here can be completely removed since Elixir is a core Docker image.

@stevenzeck
Copy link
Contributor

Looks like Elixir 1.6 is available as of a couple of weeks ago, so native formatting is built into it.

@Glavin001
Copy link
Member

And the build is currently failing after said update 😝 : https://hub.docker.com/r/unibeautify/exfmt/builds/bwrhhexwcjbcaqcz6erpvab/

�[91m** (Mix) You're trying to run :exfmt on Elixir v1.5.2 but it has declared in its mix.exs file it supports only Elixir ~> 1.6-dev

@stevenzeck
Copy link
Contributor

It's been failing for a long time because the base Docker image for Elixir had always been 1.5. But doesn't matter anymore because formatting is built into the base Docker image now.

@Glavin001
Copy link
Member

But doesn't matter anymore because formatting is built into the base Docker image now.

Failing build doesn't matter? 😝 Sounds like the fix is just to simplify the image 👍 .

@stevenzeck
Copy link
Contributor

I guess what I'm saying is we don't need that image anymore, so I would say just remove that Dockerfile entirely. https://hexdocs.pm/mix/master/Mix.Tasks.Format.html#content

@Glavin001
Copy link
Member

The value in the Docker containers remains, as they are isolated and easily installable.

For example, I do not use Elixir, however I usually want to test every beautifier for Atom-Beautify. I use the Docker containers to quickly install these beautifiers. Otherwise, I would have to install Elixir or Mix or whatever is required. Furthermore, if I wished to uninstall it may be non-trivial to delete all of the files. Docker containers make even uninstallation trivial.

@stevenzeck
Copy link
Contributor

Just read a little deeper, 1.6 is in RC stage right now, so 1.5.3 is still the "latest" version. Apart from building from source, the build for that Dockerfile will continue to fail.

I understand and agree with what you're saying about the Docker containers. In this instance though, the official Elixir Docker image will be better suited for beautification of Elixir code. The author of exfmt also says that it will be redundant once 1.6 is released. So instead of pulling from unibeautify/exfmt, you would just do docker pull elixir.

@kumekay
Copy link
Contributor Author

kumekay commented Jan 19, 2018

Finally 1.6 is released, I'll make an update this weekend
https://elixir-lang.org/blog/2018/01/17/elixir-v1-6-0-released/

@stevenzeck
Copy link
Contributor

Yea, but I don't think there is a Docker image for us to continue with. The new official Elixir 1.6 images, like those here https://store.docker.com/images/elixir, will have those features built right into them.

@Glavin001
Copy link
Member

@szeck87 We need a Docker image which runs the format command directly. The beautifier image simply being a wrapper with additional info including:

LABEL io.whalebrew.name 'exlixir_format'
LABEL io.whalebrew.config.working_dir '/workdir'
WORKDIR /workdir

ENTRYPOINT ["mix", "format"]
CMD ["--help"] # <-- May not work?

Running this image calls mix format YOUR_ARGS instead of simply YOUR_ARGS.

@stevenzeck
Copy link
Contributor

docker run --rm -it -v $(pwd):/app -w /app elixir:1.6-slim mix format test.ex [args] does the job from the base Elixir image. --help won't work as an arg, mix help format is the way to do it.

@Glavin001
Copy link
Member

OK I think you're right. I was concerned about the arguments for Atom-Beautify's "Executables". For Atom-Beautify's current implementation of "Executables" it assumes both Docker and non-Docker installations of beautifiers use the same arguments.

However, the Executable is mix! This is what I was somehow forgetting! https://hexdocs.pm/mix/Mix.Tasks.Format.html
We are no longer using another formatter executable, instead it is mix coupled with the format options! Needs to be changed at Glavin001/atom-beautify#1738 (comment) 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants