Skip to content
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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Enable eslint-jsdoc #12019

wants to merge 9 commits into from

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Jun 4, 2024

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.

  • Uses the recommended ruleset. In the future, we may want to turn on TS mode, but for now we'll default to the recommend for JS-only.
  • Disable rules that are against our coding consistencies, at least for the time being and added comments why
  • Disabled quite a few rules which require manual fixes, like filling in types and descriptions, until we can manually fix.
  • Autofix with npm run eslint -- --fix

Issue number and link

Fixes #10815

Testing plan

  1. Run eslint and ensure there are no failures
  2. Run build-docs and ensure the built documentation is not broken
  3. Run build-ts and ensure there are no errors (there will still be warnings)

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@ggetz ggetz requested a review from jjspace June 4, 2024 19:05
Copy link

github-actions bot commented Jun 4, 2024

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.
*
Copy link
Contributor

@jjspace jjspace Jun 7, 2024

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.

Comment on lines +88 to +89
* @param faceResolution
* @param halfWidthMeters
Copy link
Contributor

@jjspace jjspace Jun 7, 2024

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? 🤔

@jjspace
Copy link
Contributor

jjspace commented Jun 7, 2024

If I could summarize the changes I'm seeing

  1. rename properties that are synonyms to the "true" one, this is good imo 👍
    • constructor to class
    • return to returns
    • exception to throws
  2. remove empty lines as outlined in my comment above 😐
    • I agree with some of these that were excessive (double lines, ones between some tags)
    • I disagree with many of them that created valuable visual separation between chunks of tags and between the description and all tags. again, mentioned above
  3. adding @params for those that didn't exist before, again see above 🤷

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Evaluate eslint-plugin-jsdoc
2 participants