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

Implement eslint.rules.customizations - with overrides #1164

Merged
merged 12 commits into from
Apr 6, 2021
Merged

Implement eslint.rules.customizations - with overrides #1164

merged 12 commits into from
Apr 6, 2021

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Jan 23, 2021

Continues @jamesorlakin's great work from #993, with the changes requested by @dbaeumer to take settings as an array and allow ignoring rules.

Fixes #468 based on the discussions in #561.

Fixes #1199.

In the demo, note the underline under bar and console.log changing to red (error):

Screen.Recording.2021-03-12.at.12.38.53.PM.mov

Co-authored-by: James Lakin contact@jameslakin.co.uk

README.md Outdated
@@ -146,6 +146,29 @@ This extension contributes the following variables to the [settings](https://cod
- `all`: fixes all possible problems by revalidating the file's content. This executes the same code path as running eslint with the `--fix` option in the terminal and therefore can take some time. This is the default value.
- `problems`: fixes only the currently known fixable problems as long as their textual edits are non overlapping. This mode is a lot faster but very likely only fixes parts of the problems.

- `eslint.rules.customizations`: force rules to report a different severity within VS Code compared to the project's true ESLint configuration. This is an array that allows two kinds of glob patterns:
- `"override`": Overrides for rules with names that match, factoring in asterisks: `{ "override": "no-*", "severity": "warn" }`
- `"reset"`: Excludes rules matching a glob from previous overrides: `{ "reset": "*jsx*" }`
Copy link
Member

Choose a reason for hiding this comment

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

Can you please provide an example on when users should use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I did, in the next example. Do you mean something else @dbaeumer?

@@ -1290,6 +1348,12 @@ function validate(document: TextDocument, settings: TextDocumentSettings & { lib
}
}

const newRuleCustomizationsKey = JSON.stringify(settings.rulesCustomizations);
Copy link
Member

Choose a reason for hiding this comment

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

I would actually hash this to make it clear that we use the content as a key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Err, is there a particular hash function you're looking for here? I'm under the impression JSON.stringify works as a rudimentary, though space-inefficient, hash.

@dbaeumer
Copy link
Member

dbaeumer commented Feb 8, 2021

Add my comment. Again thanks for working on this.

@dbaeumer
Copy link
Member

@JoshuaKGoldberg anything I need to do to move this forward?

@JoshuaKGoldberg
Copy link
Contributor Author

Ah! It fell off my radar. Looking again now -- thanks for the ping!

@stenzengel
Copy link

Can I help? E.g. is it easily possible to test this PR before it is merged or a new version is released? (Unfortunately I have no experience with VS Code extension development, yet.)

@dbaeumer
Copy link
Member

@stenzengel To test things you can do the following:

  • clone @JoshuaKGoldberg fork
  • npm install
  • open in VS Code
  • go to the debugger tab
  • run Launch Client

That launches a new VS Code instance which uses the ESLint extension from source.

@stenzengel
Copy link

@JoshuaKGoldberg
I have only done a quick test, but my use case works like a charm so far. I will continue testing.

One short question: I'm not sure, if you prefer the property name "override" to my original proposal "severity" or if it is an oversight. I am not a native speaker, so I am not sure which one sounds better.

Current implementation:

{ "rule": "*",         "override": "downgrade" },
{ "rule": "*console*", "override": "reset" },

Original proposal

{ "rule": "*",         "severity": "downgrade" },
{ "rule": "*console*", "severity": "reset" },

@dbaeumer
Copy link
Member

I prefer severity over override

@JoshuaKGoldberg
Copy link
Contributor Author

This is how it is now:

  "eslint.rules.customizations": [
    { "override": "no-*", "severity": "info" },
    { "override": "!no-*", "severity": "downgrade" },
    { "reset": "radix", "severity": "reset" }
  ]

Is that acceptable to both of you?

@stenzengel
Copy link

I don't understand yet in which case exactly you have to use the property name "reset" instead of "override". Only in cases where the value for "severity" is also "reset"?. I also don't see any commit yet where I could hava a closer look.

I definitely don't want you to make a change to your code that you're not completely happy with because of my configuration proposition. I.e. for me all configuration alternatives are ok as long as it is possible to downgrade all rules.

@JoshuaKGoldberg
Copy link
Contributor Author

Oh you know what, I had just forgotten to update the README.md example 😅 . Sorry! Yes, I'll switch to this -- your proposal @stenzengel:

  "eslint.rules.customizations": [
    { "rule": "no-*", "severity": "info" },
    { "rule": "!no-*", "severity": "downgrade" },
    { "rule": "radix", "severity": "reset" }
  ]

@dbaeumer
Copy link
Member

One minor suggestion: I would like to rename reset since to my undestanding it will use the value defined in the eslintrc file. Suggestions are: default since it is the value in the eslintrc file or eslintrc to make this even more clear. I actually woul prefer default.

@dbaeumer
Copy link
Member

@JoshuaKGoldberg is this ready for another review?

@JoshuaKGoldberg
Copy link
Contributor Author

@dbaeumer yes please!

Copy link
Member

@dbaeumer dbaeumer left a comment

Choose a reason for hiding this comment

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

Very nice work. Only minor things. Thanks again for doing this.

@JoshuaKGoldberg JoshuaKGoldberg requested a review from dbaeumer April 1, 2021 13:16
@dbaeumer dbaeumer merged commit e4b2738 into microsoft:main Apr 6, 2021
@dbaeumer
Copy link
Member

dbaeumer commented Apr 6, 2021

Merged !!

@dbaeumer dbaeumer added this to the 2.1.20 milestone Apr 6, 2021
@dbaeumer
Copy link
Member

dbaeumer commented Apr 6, 2021

I drafted a pre-release here: https://github.com/microsoft/vscode-eslint/releases/tag/insider%2F2.1.20 in case you want to give it a try.

@JoshuaKGoldberg JoshuaKGoldberg deleted the eslint-overrides-2 branch April 6, 2021 13:27
@JoshuaKGoldberg
Copy link
Contributor Author

Whoohoo! 🙌

Thanks everyone for helping push this through! It feels like we made a real team effort and I'm stoked to start adding this config to repositories. 🚀

@stenzengel
Copy link

Thanks a lot, @JoshuaKGoldberg and @dbaeumer . The pre-release worked for me without any problems.

@lolmaus
Copy link

lolmaus commented Apr 7, 2021

Hi!

I have installed v2.1.20, added this to my settings.json:

    "eslint.rules.customizations": [
        {
            "rule": "*", "override": "info"
        }
    ],

I expect the linting problems to be underlined yellow, but they are still red. What am I doing wrong?

@dbaeumer
Copy link
Member

dbaeumer commented Apr 7, 2021

@lolmaus the correct syntax is:

	"eslint.rules.customizations": [
		{ "rule": "*", "severity": "info" }
	]

@lolmaus
Copy link

lolmaus commented Apr 7, 2021

Ah, thanks! My syntax is correct, but I used the keyword override from this PR's screenshot. The correct keyword is severity.

Thank you for the absolutely awesome feature! 🙇‍♂️🙏

@lolmaus
Copy link

lolmaus commented Apr 7, 2021

Here's a config that sets the color of fixable rules to blue, non-fixable to yellow:
https://gist.github.com/lolmaus/e1be5655288fca67e53a22b720adf40d

Includes eslint, @typescript-eslint and prettier.

@lolmaus
Copy link

lolmaus commented Apr 8, 2021

image

I love how different severities overlap and you can now see an important problem inside an unimportant problem! 😍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants