Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
docs(eslint-plugin): standardize rule description format#4976
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
docs(eslint-plugin): standardize rule description format#4976
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nx-cloudbot commentedMay 15, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the PR,@Josh-Cena! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day. |
netlifybot commentedMay 15, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site settings. |
| [`@typescript-eslint/semi`](./semi.md) | Require or disallow semicolons instead of ASI | | :wrench: | | | ||
| [`@typescript-eslint/space-before-function-paren`](./space-before-function-paren.md) | Enforces consistent spacing before function parenthesis | | :wrench: | | | ||
| [`@typescript-eslint/space-infix-ops`](./space-infix-ops.md) | This rule is aimed at ensuring there are spaces around infix operators. | | :wrench: | | | ||
| [`@typescript-eslint/space-before-blocks`](./space-before-blocks.md) | Enforce consistent spacing before blocks | | :wrench: | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There are some rules missing in this file. I assume it's simply an oversight.
bradzacherMay 16, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
there shouldn't be any missing files - it should be tested against and enforced.
or maybe this readme file isn't tested like the main readme is...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think it's tested as comprehensively as the main readme, no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should definitely fix this! Considering this file powers our website - it should be generated and tested just like the one in the main readme!
Or perhaps we should just generate the website table at website build time instead of making it rely on a markdown file on disk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Certainly. I've addeddocs/rules/README.md
to the tests.
we should just generate the website table at website build time
We can do that later; it's a bit harder.
codecovbot commentedMay 15, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #4976 +/- ##==========================================+ Coverage 91.32% 93.83% +2.50%========================================== Files 132 286 +154 Lines 1487 9838 +8351 Branches 224 2939 +2715 ==========================================+ Hits 1358 9231 +7873- Misses 65 328 +263- Partials 64 279 +215
Flags with carried forward coverage won't be shown.Click here to find out more.
|
@@ -1,6 +1,6 @@ | |||
# `array-type` | |||
Requires using either `T[]` or `Array<T>` for arrays. | |||
Require using either `T[]` or `Array<T>` for arrays. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, interesting! I was actually thinking of going the other way and preferring third-person singular."Requires XYZ" and such.
Reasoning: imperative mood reads (to me, at least) as a command: that you should do the thing in the sentence. That's only true for some rules, and then within each rule not true for all projects.
OTOH third-person singular indicates it's describing the rule. That's more directly the goal of these description strings & docs.
For reference,https://eslint.org/docs/rules uses imperative but the individual pages such ashttps://eslint.org/docs/rules/array-callback-return use third-person singular.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, maybe I should line up the format with ESLint's, then? Using third-person singular for individual pages.
As for the "command-sounding" tone, though, the imperative mood reads like a description as well. "Require"/"Disallow" etc. sound obvious enough that the omitted subject is "the rule", instead of "you must do X".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah I'm thinking it would make sense for ESLint to use third-person singulareverywhere 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
FWIW, I like saving one character😥
f8b7b7f
to88c3061
CompareMerge conflicts, merge conflicts... 😥 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is looking good to me
I'll leave it to our resident documentation master@JoshuaKGoldberg for the final stamp.
I'm a bit swamped with PR things at the moment, but should get through my backlog... soon! This week! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fantastic, thanks! This standardization will be super helpful as we continue to automate the site.
text: `${rule.meta.docs?.description.replace( | ||
/(?<!`)(require|enforce|disallow)/gi, | ||
'$1s', | ||
)}.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Continuing the thread from#4976 (comment): I'd really rather we didn't do the extra work to massage between one and two. In addition to requiring (admittedly clever) tricks like this one, I get a bit of cognitively dissonance havingslightly different descriptions for the same rule in multiple places.
It's fine having this here for now, so we can just move this discussion to#5096 as a followup if you feel strongly.
Josh-CenaMay 29, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree. I also mentioned in#5085 (comment) I'd like this to be eventually automated as well. However, I do prefer the table to be imperative because it looks more... succinct & professional? My own feeling when reading the ESLint docs.
PR Checklist
Overview
Right, so here's some unwieldy chores.
enforce
/require
/disallow
—following ESLint format.Should we add it to the tests that descriptions must start with
enforce
/require
/disallow
?