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

docs: Give advice on when to use warn #16753

Closed
wants to merge 1 commit into from

Conversation

jfmengels
Copy link
Contributor

@jfmengels jfmengels commented Jan 6, 2023

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[X] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

I added an explanation of when to use the "warn" severity, as I often feel like it's being misused and misunderstood. I asked what they were used for in this Twitter thread and @nzakas replied

Warnings were designed to aid in transitioning to using new rules. Mark as a warning to let people know this rule will become an error in the future without breaking the build now. It’s intended as a temporary measure.

which I used as the basis of the explanation.

Is there anything you'd like reviewers to focus on?

I have been using and contributing to ESLint and linters for years, and had never known that this was the intent behind them. I imagine this will actually surprise people. I for a long time thought it was about more or less important rules.

I don't necessarily agree that warnings are a good use for this considering that it will easily lead a large wall of errors, which is why I have added some advice on keeping the number of these issues low.

I think it's important to give good advice here so I'm open to suggestions to improve the message.

@jfmengels jfmengels requested a review from a team as a code owner January 6, 2023 16:22
@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon documentation Relates to ESLint's documentation labels Jan 6, 2023
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 6, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: jfmengels / name: Jeroen Engels (ee9b3d1)

@netlify
Copy link

netlify bot commented Jan 6, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit ee9b3d1
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/63b84ae16c1c13000858ec58
😎 Deploy Preview https://deploy-preview-16753--docs-eslint.netlify.app/user-guide/configuring/rules
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@@ -20,6 +20,8 @@ To change a rule's severity, set the rule ID equal to one of these values:
* `"warn"` or `1` - turn the rule on as a warning (doesn't affect exit code)
* `"error"` or `2` - turn the rule on as an error (exit code is 1 when triggered)

The `"warn"` severity is meant to be used as a temporary measure. It can be used to enable a rule so that new reported issues can be addressed, without breaking the build due to existing ones. It is recommended to keep the number of these issues low so that new issues get more easily noticed and don't get ignored.
Copy link
Member

Choose a reason for hiding this comment

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

I think we haven't decided yet whether this should be the only advice for using warnings: #16696

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I hadn't seen that issue. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel we can add usually here to address that this is not with every case but most of the cases.

Suggested change
The `"warn"` severity is meant to be used as a temporary measure. It can be used to enable a rule so that new reported issues can be addressed, without breaking the build due to existing ones. It is recommended to keep the number of these issues low so that new issues get more easily noticed and don't get ignored.
The `"warn"` severity is usually meant to be used as a temporary measure. It can be used to enable a rule so that newly reported issues can be addressed, without breaking the build due to existing ones. It is recommended to keep the number of these issues low so that new issues get more easily noticed and don't get ignored.

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'm fine with that, but then I think we should also indicate the other potential uses (just like @bmish said in a comment below)

Copy link
Member

Choose a reason for hiding this comment

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

Let's not make this change piece-by-piece. I'll take a stab at writing up what I think is the best advice for using "warn".

@bmish
Copy link
Sponsor Member

bmish commented Jan 6, 2023

I'm in favor of mentioning several potential/common use cases for warnings like this, as well as hinting at potential drawbacks of warnings like becoming noisy and easily ignored.

@snitin315 snitin315 added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jan 8, 2023
nzakas added a commit that referenced this pull request Feb 10, 2023
@github-actions
Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Feb 12, 2023
@nzakas
Copy link
Member

nzakas commented Feb 13, 2023

We have another PR that will close this one when merged: #16882

mdjermanovic pushed a commit that referenced this pull request Feb 13, 2023
* docs: Add explanation of when to use 'warn' severity

Fixes #16696
Closes #16753

* Update docs/src/use/configure/rules.md

Co-authored-by: Nitin Kumar <snitin315@gmail.com>

---------

Co-authored-by: Nitin Kumar <snitin315@gmail.com>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Aug 13, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants