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 elm-format as a new fixer #916

Merged
merged 5 commits into from Sep 9, 2017
Merged

Add elm-format as a new fixer #916

merged 5 commits into from Sep 9, 2017

Conversation

soywod
Copy link
Contributor

@soywod soywod commented Sep 9, 2017

Add a new fixer: elm-format

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. 👍 See my comments here.

return ale#node#FindExecutable(a:buffer, 'elm_format', [
\ 'node_modules/.bin/elm-format',
\ 'node_modules/elm-format/bin/elm-format',
\ 'node_modules/elm-format/index.js',
Copy link
Member

Choose a reason for hiding this comment

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

For at least this file, you will have to execute it with node on Windows, or it could lead to the .js file being opened in a text editor or similar. Look at the code for the eslint fixer for an example.

Copy link
Contributor Author

@soywod soywod Sep 9, 2017

Choose a reason for hiding this comment

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

I made a mistake here, I just had a look on this file and it's like an installer, so I will remove it. The 'node_modules/.bin/elm-format' is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

" Description: Integration of Elm with ALE.

call ale#Set('elm_format_executable', 'elm-format')
call ale#Set('elm_format_use_global', 1)
Copy link
Member

Choose a reason for hiding this comment

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

Set this to 0 by default, to match the other options. Local versions should be tried first, then global versions, by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@w0rp
Copy link
Member

w0rp commented Sep 9, 2017

Oh yes, don't forget to document the new options too.

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Cool. Looks fine to me.

@w0rp w0rp merged commit f3da8f4 into dense-analysis:master Sep 9, 2017
@w0rp
Copy link
Member

w0rp commented Sep 9, 2017

Cheers! 🍻

@soywod soywod deleted the elm-format-fixer branch September 10, 2017 16:41
cwonrails added a commit to cwonrails/ale that referenced this pull request Sep 10, 2017
* 'master' of git://github.com/w0rp/ale:
  Explain the table of contents script better, and simplify it a bit
  Fix numerous issues with integration documentation tags and the table of contents, and add a script to check for theses issues
  Move scripts for tests into the test directory, and do not export the Batch script for running tests
  Elm local install support (dense-analysis#915)
  Fix an SML variable init bug, and get the SML cm file tests to pass on Windows
  Get the TSLint handler tests to pass on Windows
  Simplify some Rust handler code, and get the Rust handler tests passing on Windows
  Add elm-format as a new fixer (dense-analysis#916)
  Fix some path issues, and get lsp dir tests passing on Windows
  execute the `set encoding` test setting to get Vint to shut up
  Fix more random Windows test issues
  dense-analysis#917 Cover the old _args option for flake8 with a test, just in case
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.

None yet

2 participants