-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Repo: Enforce that all rules have a "When Not To Use It" docs section? #7257
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
Comments
We probably need to do a broader review of the docs to clean them up. I'm in favour of us adding it to the template as a section that would be good to include - but I don't think saying "always" is necessarily correct. |
Are there any rules where you don't think it'd be valuable? I think from at least a philosophical perspective, it'd be beneficial for us to showcase that all rules are nuanced. A lot of new-to-ESLint devs treat rules as immutable truths, and it's good to encourage them to think more deeply on them. Sent #7447 as a draft for reference of a rough attempt to fill out all remaining ones. |
commented with some quick examples |
Cool - I'm going to say this has passed triage, in that the When Not To Use It examples should be filled out more. I'll try to get to that draft PR to fix up for the comments. +1 to @Josh-Cena's #7447 (comment) that mentioning using disable comments there can be valuable. |
One way to frame this - IMO if the rule is in our recommended set - there probably shouldn't be a "when not to use it" section. If we recommend the rule we believe it forms the base set of checks that every TS codebase should have no - we don't believe there is a reason you should turn it off! |
I've had non-zero problems with no-var-requires and JSON files (or Webpack in general, when you want to import hundreds of images without writing them on separate lines). Similarly, I appreciate these are edge cases that maybe not everyone encounters, but we can definitely find a way to generalize them. |
The webpack stuff - 100% outside our concern. This is becoming more and more of a true assumption as projects move to be esm-only nodejs (eg Essentially the rule is right - if you're writing modern TS you should use es6 module syntax only. If your environment happens to support The only time I've seen it "not lazy" and actually required is the |
Coming back to this!
I would flip that logic: if the rule isn't in the recommended set, then IMO there should be a "when not to use it" section. But if the rule's in recommended, that means a lot more eyes are going to be on it. And we've had issues with devs thinking the recommended ruleset is all must-have. https://typescript-eslint.io/rules/no-floating-promises is a good example of "this absolutely should be recommended, but dang do people need caveats on how to use it right". |
Suggestion
Right now, about 70 rule docs files have a
## When Not To Use It
heading. Is this practice something we want to standardize for all rules and enforce as a CI/lint/test check?FWIW I'd be in favor. I think it's good to always portray a nuanced view of when you would or wouldn't want to use a rule.
The text was updated successfully, but these errors were encountered: