-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Automate docs with eslint-doc-generator #3469
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3469 +/- ##
=======================================
Coverage 97.57% 97.57%
=======================================
Files 129 129
Lines 9192 9192
Branches 3331 3331
=======================================
Hits 8969 8969
Misses 223 223 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
1a69655
to
15c0f53
Compare
@@ -276,12 +276,6 @@ module.exports = { | |||
schema: [{ | |||
type: 'object', | |||
properties: { | |||
customValidators: { |
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 option appears to be unused in this rule. eslint-doc-generator caught this because it checks that all rule options are mentioned in the rule doc.
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.
It's used by the propTypes util, so even if it's missing documentation, it's still used.
The docs are never the source of truth, only the code is, so that "catch" seems a bit backwards.
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 also checked the tests for this rule and didn't see the option tested. That seems like an issue. I'll add the option back.
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.
Indeed, missing tests is far more worrisome than missing docs, but either way removing the option could break people, so we'll need to add the missing tests/docs :-) thanks for noticing!
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.
Added the option back and mentioned it in the docs. Someone should follow-up separately to add the missing test.
|
||
<!-- end rule header --> | ||
|
||
Please use the `"beforeSelfClosing"` option of the [jsx-tag-spacing](https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-tag-spacing.md) rule instead. |
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.
it'd be better to keep this attached to the deprecation message if possible
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.
While the standard meta.replacedBy
rule field allows the replacement rule to be indicated, there's no way for us to indicate a "replacement option" using the rule metadata. So currently there's no practical means for including this in the autogenerated content. Personally, it seems fine to me if some additional information about the deprecation comes later in the rule doc. Some people might want to expand on the deprecation with an entire paragraph anyway.
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.
hmm - standard wrt eslint itself?
Perhaps replacedBy
could be extended to also support an object, with "rule" and "notes" or something?
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.
meta.replacedBy
is defined as string array here:
- https://github.com/eslint/eslint/blob/4715787724a71494ba0bb0c5fe4639570bb6985b/lib/shared/types.js#L151
- https://github.com/typescript-eslint/typescript-eslint/blob/733b3598c17d3a712cf6f043115587f724dbe3ef/packages/utils/src/ts-eslint/Rule.ts#L74
- https://eslint.org/docs/latest/developer-guide/working-with-rules
So while I do think a "deprecation message" field would generally be useful and could be used for the purpose you suggest, it would take a concerted effort to propose and update various definitions and tooling to handle objects like that. For now, I recommend we just expand on deprecations later in the rule doc.
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.
Sounds good, but it'd be great to pursue making that change :-)
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 could file an issue in ESLint suggesting it. I hope it wouldn't block this PR.
Here's a potential API allowing deprecation messages about each specific replacement rule, or a general deprecation message (not specific to any replacement). Let me know if you like it.
replacedBy: ["replacement-rule-name"]
replacedBy: [{ name: "replacement-rule-name" }]
replacedBy: [{ name: "replacement-rule-name", message: "some deprecation message specific to this replacement" }]
replacedBy: [{ message: "general deprecation message" }]
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.
It wouldn’t block it. Your suggested schema seems awesome and backwards compatible.
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 finally wrote up a more detailed proposal for this:
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.
Put together the RFC!
@bmish btw i failed to mention this in the main comment of my review, but thank you, this is awesome! |
2325184
to
147df5f
Compare
de26e0f
to
484f40d
Compare
💡 Manually fixable by [editor suggestions](https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions).\ | ||
❌ Deprecated. | ||
|
||
| Name | Description | 💼 | 🔧 | 💡 | ❌ | |
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.
what's the deal with all these nonbreaking spaces? i don't think markdown tables require them
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 minimizes line-breaking/word-wrapping in the rule name column so that we can see complete rule names more easily.
Context: bmish/eslint-doc-generator#110
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.
oof, ok - can they be actual nonbreaking spaces instead of html ones, or will markdown parsers break on that?
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.
Interesting idea, I'll give that a try.
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'll merge this as-is; if that can be made cleaner then I'd appreciate a followup :-)
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.
It appears to work: bmish/eslint-doc-generator#172
The good news is that you'll receive any improvements like this when future versions of the tool are released.
484f40d
to
f83b388
Compare
I built this CLI tool eslint-doc-generator for automating the generation of the README rules list and rule doc title/notices for ESLint plugins. It follows common documentation conventions from this and other top ESLint plugins and will help us standardize documentation across ESLint plugins (and generally improve the usability of our custom rules through better documentation and streamline the process of adding new rules).
This is a follow-up to the work I did to add the original notices in #3359 #3361 #3364 #3362. eslint-doc-generator is significantly more robust compared to the previous documentation generator script/tests which I have now removed. It has additional functionality (for example, the rules list legend is auto-generated, and the rules list will show additional columns of information when applicable) and 100% test coverage.
There are some discrepancies between the old docs and the new docs. Most of these are intentional and likely improvements. If you don't like any of the changes, let me know and I may be able to tweak eslint-doc-generator or add a config option to support the desired behavior.