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

Passthrough eslint config to prettier-eslint #854

Closed

Conversation

morhetz
Copy link
Contributor

@morhetz morhetz commented Aug 16, 2017

closes #853

@morhetz morhetz changed the title WIP Passthrough eslint config to prettier-eslint Passthrough eslint config to prettier-eslint Aug 16, 2017
@w0rp
Copy link
Member

w0rp commented Aug 16, 2017

I'll close this for now, as no release of the tool implements this argument.

@w0rp w0rp closed this Aug 16, 2017
@w0rp
Copy link
Member

w0rp commented Aug 16, 2017

Oh, 4.2.0 does, as of less than half an hour ago.

I'll reopen this. What you should do is leave this off by default, and add an option for turning this behaviour on. Then edit the documentation to explain this. It can be turned on by default after enough people upgrade.

@w0rp w0rp reopened this Aug 16, 2017
call ale#Set('javascript_prettier_eslint_executable', 'prettier-eslint')
call ale#Set('javascript_prettier_eslint_use_global', 0)
call ale#Set('javascript_prettier_eslint_options', '')

function! ale#fixers#prettier_eslint#GetExecutable(buffer) abort
return ale#node#FindExecutable(a:buffer, 'javascript_prettier_eslint', [
\ 'node_modules/prettier-eslint-cli/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.

Are there older versions of prettier-eslint-cli with this path? If so, leave this in, but move it down the List.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. There were no version with this path

Copy link
Member

Choose a reason for hiding this comment

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

Okay, cool. 👍

@morhetz
Copy link
Contributor Author

morhetz commented Aug 16, 2017

@w0rp

What you should do is leave this off by default, and add an option for turning this behaviour on. Then edit the documentation to explain this. It can be turned on by default after enough people upgrade.

Hm, this fixer was never working as it supposed to due to wrong exec path and lack of eslint config passing. Best case it was working just as plain prettier.js formatter having no eslint --fix functionality. So there are zero to none users of it.

Wouldn't it be better/easier to engage users to upgrade prettier-eslint-cli package?

@morhetz
Copy link
Contributor Author

morhetz commented Aug 16, 2017

Leaving this option off by default would still keep fixer broken and unusable

@w0rp
Copy link
Member

w0rp commented Aug 16, 2017

If you set the argument by default, then the integration will stop working for people who are using the plugin who haven't yet upgraded prettier-eslint-cli.

@w0rp
Copy link
Member

w0rp commented Aug 16, 2017

New options for any tool can't be used by default until some time has passed and people have the opportunity to upgrade first.

@morhetz
Copy link
Contributor Author

morhetz commented Aug 16, 2017

Integration is not working without that option

@morhetz
Copy link
Contributor Author

morhetz commented Aug 16, 2017

It's not just a new one, it's core functionality of prettier-eslint

@w0rp
Copy link
Member

w0rp commented Aug 16, 2017

Maybe an exception could be made here, as the configuration file is pretty important.

@w0rp
Copy link
Member

w0rp commented Aug 16, 2017

I think let's go with it. Update the documentation to indicate the minimum supported version, and cover the new argument with Vader tests.

@w0rp
Copy link
Member

w0rp commented Aug 16, 2017

I would rather there weren't so many tools for doing essentially the same thing. Personally, I wish that standard, xo, prettier-standard, and prettier-eslint didn't exist. eslint with eslint-plugin-prettier is probably the best option. Running eslint --fix followed by prettier is probably the second best option.

Still, some people like this, so it's worth supporting.

@morhetz
Copy link
Contributor Author

morhetz commented Aug 16, 2017

As far as I know eslint-plugin-prettier would require eslint-config-prettier to disable all format conflicting rules on the eslint side
prettier-eslint gives opportunity to use your preffered code style with no change
more like a drop in replacement for the eslint --fix inferring formatting options of the eslintrc

@morhetz
Copy link
Contributor Author

morhetz commented Aug 18, 2017

@w0rp could you please review when you have a time?

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 so far. Now you can add some Vader tests to cover the command and the new option.

@w0rp
Copy link
Member

w0rp commented Aug 30, 2017

I have added tests for this and merged this now. I also had to fix an issue where an empty configuration filename would sometimes be used.

@w0rp w0rp closed this Aug 30, 2017
@w0rp
Copy link
Member

w0rp commented Aug 30, 2017

Cheers! 🍻

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.

Prettier-eslint fixer not using eslintrc
2 participants