-
-
Notifications
You must be signed in to change notification settings - Fork 32k
meta: enable eslint jsdoc rules #58521
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
base: main
Are you sure you want to change the base?
Conversation
273c1fb
to
1706d82
Compare
7c5384a
to
cb6ed80
Compare
cb6ed80
to
4165c4f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #58521 +/- ##
==========================================
- Coverage 90.25% 90.08% -0.17%
==========================================
Files 635 640 +5
Lines 187635 188576 +941
Branches 36861 36944 +83
==========================================
+ Hits 169347 169885 +538
- Misses 11055 11419 +364
- Partials 7233 7272 +39
🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
cc @LiviaMedeiros @jasnell can you re-review? |
/** | ||
* @class | ||
*/ | ||
class CustomEvent extends Event { |
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.
Is this needed? What's the point of it?
/** | |
* @class | |
*/ | |
class CustomEvent extends Event { | |
class CustomEvent extends Event { |
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 not sure. One of the rules probably asked me to add it.
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.
Well let's remove it 😅 I've checked, the linter doesn't complain without it
@@ -1,5 +1,5 @@ | |||
/** | |||
* @fileoverview Check that common.skipIfEslintMissing is used if | |||
* @file Check that common.skipIfEslintMissing is used if | |||
* the eslint module is required. |
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.
* the eslint module is required. | |
* the eslint module is required. |
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 100% agree but it's against our CONTRIBUTING.md, since it's allowed in our formatter and linter right now. This should be handled through a formatter. There is a particular sentence in the document about: "Anything that can be automated should be automated"
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.
Wouldn't jsdoc/check-line-alignment
enforce 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.
This is the first time I hear about it. Perfect. Thanks! I'll enable it now. Hopefully it will fix it.
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 enabled it but it didn't fix this.
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
I propose enabling jsdoc eslint rules. This would help a lot for maintainance, and help create a more consistent code base. Currently, we have jsdoc in some areas but not in all places. Additionally, this would help folks maintaining
@types/node
package a lot.