prettier / eslint-config-prettier

Turns off all rules that are unnecessary or might conflict with Prettier.
MIT License
5.43k stars 255 forks source link

add support for `@stylistic/eslint-plugin` #272

Open abrahamguo opened 10 months ago

abrahamguo commented 10 months ago

Since formatting rules have been deprecated from core ESLint and moved to @stylistic/eslint-plugin, this PR adds support for all the same rules in @stylistic/eslint-plugin. Despite its name, @stylistic/eslint-plugin still has rules that can be used with prettier:

This PR also updates package-lock.json to lockfileVersion: 3 (the version used by the current versions of npm) and ignores the .idea folder created by JetBrains IDEs.

lydell commented 10 months ago

Hi!

Can you describe what your motivating use case is for this?

abrahamguo commented 10 months ago

Sure thing! I'd like to be able to use the stylistic rules from @stylistic/eslint-plugin that are unaffected by Prettier (the specific rules are listed above), while still using this to turn off all rules that conflict with Prettier.

I think this makes sense from a comprehensive purpose — since eslint-config-prettier disables rules from eslint, @typescript-eslint, and eslint-plugin-react, and those rules have been moved (in eslint) or copied (in @typescript-eslint and eslint-plugin-react) to @stylistic/eslint-plugin, I think it makes sense that eslint-config-prettier is able to disable those same rules, no matter which package they come from.

lydell commented 10 months ago

Thanks! Just so that I understand what you’re doing correctly: Why don’t you enable just the rules from @stylistic/eslint-plugin that don’t conflict? Is this PR about having a place that lists the ones that do conflict, so people can know which ones are safe?

abrahamguo commented 10 months ago

Great question! Two answers:

  1. Yes, I think it'd be good to have an exhaustive list of the ones that do conflict, especially since the list is the same as what's in eslint, @typescript-eslint, and react.
  2. My preferred way to discover rules when I begin using a new ESLint plugin is to turn on all rules provided by that plugin, then turn off ones that I disagree with, as they report issues in my code. By adding the @stylistic/eslint-plugin rules here, I am able to do the same thing for this plugin - turn on all rules, then rest assured that all rules that conflict with Prettier, will be turned off.
lydell commented 10 months ago

Aha, turning on all rules from a plugin and then disabling the ones that conflict – I can see that being a thing. :+1:

Then I have a request: I would rather have all the @stylistic stuff duplicated than using that helper function, so we can handle deprecations correctly. I don’t mind the duplication. They are forked rules after all, and may change over time, while the rules they replace are basically frozen in time.

abrahamguo commented 10 months ago

Totally fair, makes sense! I anticipated you might comment about that, since it was a very different style than what was in that file before. I'll go ahead and do that in a bit!

abrahamguo commented 9 months ago

I've mostly completed this — I just need to figure out how to get the tests working properly. It's very confusing because there are five different stylistic plugins — js, ts, tsx, plus, and all, which contains all of the other 4.

TheDelta commented 2 months ago

Any news? Would love to have this built-in

abrahamguo commented 2 months ago

I will see if I can get this finished up!

changeset-bot[bot] commented 2 months ago

🦋 Changeset detected

Latest commit: c4651b1b78ebd90418e7a39ea356ee65ce55c11f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ---------------------- | ----- | | eslint-config-prettier | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

thomaslecoeur commented 2 months ago

Any update on the merge?

abrahamguo commented 2 months ago

No updates, I will probably need someone else to take this over. The logic is done, I simply need help finishing up the unit tests.

Further complicating this is that ESLint Stylistic plans to merge their various packages (default, JavaScript, TypeScript, JSX) into one.

lydell commented 2 months ago

For those wanting this and banging their heads against the unit tests, note:

the testing setup is overdue some simplification

https://github.com/prettier/eslint-config-prettier/issues/275#issuecomment-1868093670

(I’m not involved in the project anymore, I just occasionally read notifications on the repo.)

Kenneth-Sills commented 1 month ago

I'm making a fork of the now archived eslint-config-airbnb-typescript project, and will also need to think of a good way to add tests. If any of the work can be translated for this MR, I'll try to get this moving again.


UPDATE: Yeah, the tests could use a little love. We can get the current tests in a friendly state with this PR using a fairly simple patch (see dropdown below). Things are just failing now because the plugins aren't actually enabled in the testing pass for legacy configs, and those tests are smart enough to see they aren't present.

Click to Open ```diff diff --git a/.eslintrc.base.js b/.eslintrc.base.js index a17afc8..a01f52d 100644 --- a/.eslintrc.base.js +++ b/.eslintrc.base.js @@ -98,6 +98,15 @@ module.exports = { files: ["test-lint/{react,flowtype}.js"], parserOptions: { parser: "@babel/eslint-parser" }, }, + { + files: ["test-lint/@stylistic.js"], + extends: [ + "plugin:@stylistic/all-extends", + "plugin:@stylistic/js/all-extends", + "plugin:@stylistic/jsx/all-extends", + "plugin:@stylistic/ts/all-extends", + ], + }, ], settings: { react: { diff --git a/eslint.base.config.js b/eslint.base.config.js index a15d6f2..91d2d0a 100644 --- a/eslint.base.config.js +++ b/eslint.base.config.js @@ -89,9 +89,11 @@ module.exports = [ ...vue.configs.recommended.rules, }, }, - ...eslintrcBase.overrides.map(({ parserOptions, ...override }) => ({ - ...override, - languageOptions: { parser: require(parserOptions.parser) }, - })), + ...eslintrcBase.overrides + .filter(({ parserOptions }) => parserOptions) + .map(({ parserOptions, ...override }) => ({ + ...override, + languageOptions: { parser: require(parserOptions.parser) }, + })), { files: ["test-lint/@stylistic.js"], ...stylistic.configs["all-flat"] }, ]; ```

Mind you, this probably isn't a "proper" fix in that we probably want to make sure we have separate files for the jsx and ts rules to be tested. And we may even want separate file(s) for the core on top of that. I'll continue looking at this another day. 😄

Kenneth-Sills commented 1 month ago

I've submitted a PR against this branch here, which fixes up the tests. It applies the patch above, creates separate lint-validate-fails tests for each plugin, and adds the missing changeset.

We should be good to go soon! 🚀

JosueGalRe commented 1 week ago

Thank you for this much needed PR!

Any news on this? I can see that the PR adding tests was done some weeks ago 😿 . Having this merged is definitely something a lot of people (including myself) would love to have, especially as people migrate to @stylistic.

abrahamguo commented 5 days ago

@Kenneth-Sills merged!

Kenneth-Sills commented 5 days ago

Thank you! @JounQin, this is now ready for workflows and review. :smile: