docs(eslint-plugin): standardize rule description format #4976
Conversation
|
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 on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify 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.
There are some rules missing in this file. I assume it's simply an oversight.
There was a problem hiding this comment.
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.
I don't think it's tested as comprehensively as the main readme, no.
There was a problem hiding this comment.
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.
Certainly. I've added docs/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.
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.
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 as https://eslint.org/docs/rules/array-callback-return use third-person singular.
There was a problem hiding this comment.
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.
Yeah I'm thinking it would make sense for ESLint to use third-person singular everywhere 🤷
There was a problem hiding this comment.
FWIW, I like saving one character😥
f8b7b7f to
88c3061
Compare
|
Merge conflicts, merge conflicts... 😥 |
There was a problem hiding this comment.
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! |
| text: `${rule.meta.docs?.description.replace( | ||
| /(?<!`)(require|enforce|disallow)/gi, | ||
| '$1s', | ||
| )}.`, |
There was a problem hiding this comment.
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 having slightly 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.
There was a problem hiding this comment.
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?