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 @stylistic/eslint-plugin #272

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

abrahamguo
Copy link

@abrahamguo abrahamguo commented Dec 4, 2023

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:

  • function-call-spacing
  • lines-between-class-members
  • padding-line-between-statements
  • spaced-comment
  • jsx-curly-brace-presence
  • jsx-self-closing-comp
  • jsx-sort-props
  • lines-around-comment
  • max-len
  • no-confusing-arrow
  • no-mixed-operators
  • no-tabs
  • quotes

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
Copy link
Member

lydell commented Dec 4, 2023

Hi!

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

@abrahamguo
Copy link
Author

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
Copy link
Member

lydell commented Dec 4, 2023

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
Copy link
Author

abrahamguo commented Dec 4, 2023

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
Copy link
Member

lydell commented Dec 4, 2023

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

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
Copy link
Author

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
Copy link
Author

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
Copy link

Any news? Would love to have this built-in

@abrahamguo
Copy link
Author

I will see if I can get this finished up!

Copy link

changeset-bot bot commented Jul 14, 2024

⚠️ No Changeset found

Latest commit: 278db1d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@thomaslecoeur
Copy link

Any update on the merge?

@abrahamguo
Copy link
Author

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
Copy link
Member

lydell commented Aug 3, 2024

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

the testing setup is overdue some simplification

#275 (comment)

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

@JounQin JounQin mentioned this pull request Aug 5, 2024
@spence-s spence-s mentioned this pull request Aug 11, 2024
3 tasks
@Kenneth-Sills
Copy link

Kenneth-Sills commented Aug 27, 2024

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 --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
Copy link

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! 🚀

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.

5 participants