-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
mix format
new fixer for elixir lang
#1017
Conversation
mix format
new fixer for elixir lang
e2f8e22
to
eef9d38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks cool. See my comments here.
autoload/ale/elixir.vim
Outdated
" | ||
function! ale#elixir#Executable(buffer, executable) abort | ||
let l:elixir = ale#elixir#GetExecutable(a:buffer) | ||
if l:elixir ==# '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Travis will tell you to use is#
instead. I actually checked the difference in speed between x is# ''
and empty(x)
just now, and the former is a little faster, so I'll accept either for checking for empty strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend moving all of this code into the file for the fixer instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
autoload/ale/elixir.vim
Outdated
" Author: carakan <carakan@gmail.com> | ||
" Description: Functions for working with Elixir exec. | ||
|
||
call ale#Set('elixir_executable', '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unusual to have an executable option default to an empty string in the codebase. This option could be confused with an option for executing elixir for linting. You ought to default all executables to some name for something that can be found in PATH
.
If you need to typically run mix
instead of elixir mix
, I recommend renaming this option to something like g:ale_elixir_mix_elixir_executable
, or something. Then it should be okay for that to be an empty string by default, if mix
is typically better than elixir mix
.
Document the behavior of both options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@w0rp you're right I'll delete the elixir option.
b2c84a0
to
2f97afa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks better. 👍 Could you add the new fixer to the documentation? You'll have to add the option to the README table, the list in the main documentation file, and some you'll have to document the option you added.
(only available in elixir > 1.6)
b8936ce
to
d37b73a
Compare
d37b73a
to
be1377f
Compare
mix format
new fixer for elixir langmix format
new fixer for elixir lang
added documentation. |
Looks good! 👍 I shall merge this later. I'm getting ready to release 1.6.0. |
Cheers! 🍻 |
the idea is to add support for new code formatter from elixir 1.6 (no yet released) but is usable compiling from master repository.
Let me know any suggestions, thanks for this awesome plugin.