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 support for prettier configuration file. #886

Merged
merged 5 commits into from Aug 29, 2017
Merged

Add support for prettier configuration file. #886

merged 5 commits into from Aug 29, 2017

Conversation

aliou
Copy link
Contributor

@aliou aliou commented Aug 29, 2017

As of version 1.6.0, prettier allows passing a --config argument with a path to a configuration file.

This PR adds support for this argument when a configuration file is present and some tests inspired by the rubocop fixer tests.

@aliou
Copy link
Contributor Author

aliou commented Aug 29, 2017

For some reason the test fail on the CI but pass on my machine. 🤔 Investigating.

@aliou
Copy link
Contributor Author

aliou commented Aug 29, 2017

(Forgot to include the test prettier configuration file in the patch 😅)

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.

Adding this argument will break things for people who are using older versions. I recommend adding an option for turning this --config option on, and leaving it off by default for now until people have had the opportunity to upgrade.

@aliou
Copy link
Contributor Author

aliou commented Aug 29, 2017

Using the argument with an older version doesn't seem to break, but prints a message to stderr.

When you say adding an option, do you mean adding something like a g:javascript_prettier_use_config_file or let the user set it through the g:javascript_prettier_options ?

@w0rp
Copy link
Member

w0rp commented Aug 29, 2017

Oh, does it ignore the additional argument? I'd expect that most programs would just stop working entirely if you use an invalid argument.

@aliou
Copy link
Contributor Author

aliou commented Aug 29, 2017

Yup it ignores it on prettier 1.5.0, I haven't tried previous versions. But you're right it is probably safer to add an option in case previous versions don't ignore the argument.

What do you think about the g:javascript_use_local_config option, with 0 as a default value?

@w0rp
Copy link
Member

w0rp commented Aug 29, 2017

I think it would be best to add an option in ALE specific to prettier, like g:ale_prettier_...

@aliou
Copy link
Contributor Author

aliou commented Aug 29, 2017

Oops, yeah that's what I meant 😅

As of version 1.6.0, prettier allows passing a `--config` argument with
a path to a configuration file.
@aliou
Copy link
Contributor Author

aliou commented Aug 29, 2017

Added the g:javascript_prettier_use_local_config option and rebased to include the changes #885.

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.

This is looking better. See my comments here.


return {
\ 'command': ale#Escape(ale#fixers#prettier#GetExecutable(a:buffer))
\ . ' %t'
\ . ' ' . l:options
\ . (!empty(l:options) ? ' ' . l:options : '')
\ . (l:use_config ? ' --config ' . ale#Escape(l:config) : '')
Copy link
Member

Choose a reason for hiding this comment

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

You also need to check !empty(l:config) here.

Copy link
Member

@w0rp w0rp Aug 29, 2017

Choose a reason for hiding this comment

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

You could use && !empty(l:config) for the value of l:use_config above.

*b:ale_javascript_prettier_use_local_config*
Type: |Number|
Default: `0`

Copy link
Member

Choose a reason for hiding this comment

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

Add a short description here explaining what the option does when set to 1.

@aliou
Copy link
Contributor Author

aliou commented Aug 29, 2017

Updated.

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 fine to me.

@w0rp w0rp merged commit b36882e into dense-analysis:master Aug 29, 2017
@w0rp
Copy link
Member

w0rp commented Aug 29, 2017

Cheers! 🍻

@aliou aliou deleted the prettier-config-file-support branch August 29, 2017 15:07
rsrchboy added a commit to rsrchboy/ale that referenced this pull request Aug 30, 2017
* upstream/master:
  Make the check-supported-tools-tables script work on more machines
  Add a missing scriptencoding line
  Add support for prettier configuration file. (dense-analysis#886)
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