-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Enable eslint-jsdoc #12019
base: main
Are you sure you want to change the base?
Enable eslint-jsdoc #12019
Conversation
Thank you for the pull request, @ggetz! ✅ We can confirm we have a CLA on file for you. |
@@ -47,12 +45,10 @@ function AxisAlignedBoundingBox(minimum, maximum, center) { | |||
|
|||
/** | |||
* Creates an instance of an AxisAlignedBoundingBox from its corners. | |||
* |
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'm personally a big fan of this line break creating visual separation between a description and the tags. It looks like it's easy to enforce this with the plugin too (and auto-fixable) with the tag-lines
rule. This seems to be a vast majority of the changes this PR makes too so changing this would bring it closer to what's already there which I think implies that's our current desired style.
"jsdoc/tag-lines": [
"error",
"any",
{
startLines: 1,
},
],
Optionally it may be nice to set it or sort-tags
up for separation between specific blocks of tags (like extra lines around @example
or @see
tags) but that's a bigger change and may need some manual intervention so I'm fine not inlcuding that for now.
* @param faceResolution | ||
* @param halfWidthMeters |
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.
Without forcing the types i'm not sure these add much value . if we're leaving require-param-type
off for now then I almost think we should be turning require-param
off entirely? I don't want to bikeshed this one much so if you wanna leave it on that's fine.
Edit: That said, it doesn't actually look like there were too many of these added so it might just make sense to turn on the require-param-type
and take a bit of time to fill them out? 🤔
If I could summarize the changes I'm seeing
I've reviewed about 100 of the 800+ files and these seem to be the only changes. I'm going to stop here for now in case we change how the rules above are handled so I don't waste time reviewing files that may not actually have any changes. Overall I'm definitely a fan of including this to automate more of this consistency and look forward to reviewing and turning on more rules in the future |
Description
This PR enables eslint-plugin-jsdoc. I figured we'd go ahead and enable the defaults, and we'll turn off rules for now that will require a lot of manual fixes. We can systematically enable rules and fix them one-by-one where time allows or need arises. For example, type checking may be particularly helpful.
npm run eslint -- --fix
Issue number and link
Fixes #10815
Testing plan
eslint
and ensure there are no failuresbuild-docs
and ensure the built documentation is not brokenbuild-ts
and ensure there are no errors (there will still be warnings)Author checklist
CONTRIBUTORS.md
I have updatedCHANGES.md
with a short summary of my changeI have added or updated unit tests to ensure consistent code coverage