-
Notifications
You must be signed in to change notification settings - Fork 22
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 #243
Automate docs with eslint-doc-generator #243
Conversation
4688b0a
to
a5096cd
Compare
fe1b71d
to
25a8e12
Compare
@platinumazure I split out the lint job from the test job in CI so that we can run the lint job in only Node 18 (as you recommended in #242 (review)) as this tool requires Node >= 14. This is ready for review. |
3b36298
to
3d9b936
Compare
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.
Looks great overall! Just not sure about the changes to package scripts. Thanks for this!
package.json
Outdated
"preversion": "npm test", | ||
"update": "node ./scripts/update-rules.js", | ||
"report-coverage-html": "nyc report --reporter=html --report-dir build/coverage", | ||
"test": "nyc mocha tests/**/*.js", |
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.
Would it be possible to have npm test
still run linting and just configure CI to call unit-test and lint as appropriate?
I feel that most developers are used to having npm test
do everything CI would do. And I don't think it would be a good experience for someone to push a branch and only have linting fail at that point.
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.
@platinumazure sounds good, I reverted to having npm test
run testing and linting.
7c5b18c
to
dcaa8b1
Compare
dcaa8b1
to
9be0b57
Compare
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.
Looks great, thanks!
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 #106 #107. eslint-doc-generator is significantly more robust compared to the previous documentation generator script/tests which I have now removed. It has much more 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 can tweak eslint-doc-generator or add a config option to support the desired behavior.
I adjusted CI so that we can run the lint job in only Node 18 (as recommended in #242 (review)) as this tool requires Node >= 14.