-
-
Notifications
You must be signed in to change notification settings - Fork 163
checkDestructured
property to disable destructured checks
#532
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
Conversation
message: 'Missing JSDoc @param "bar" declaration.', | ||
}, | ||
{ | ||
message: 'Missing JSDoc @param "root0" declaration.', |
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 message is confusing, what is root0
?
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 part of the feature added in #498 --that when a destructured parameter is also missing its root declaration, that by default, the word "root" will be used followed by a 0-based auto-incrementing number for each subsequent root (one can avoid the numeric additions with specific config, or add additional items to an options array so that until all the items in the array are exhausted, it will cycle through the names, e.g., ["root", "cfg", "config"]
will add @param root
then @param cfg
, then @config0
, with the last item auto-incrementing). You could see our discussion about it in the PR for more background.
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.
Though I think it makes sense to use root0
in the fixes, subject to user configuration (disablable by setting enableRootFixer
to false
), we could give a message like "Missing JSDoc @param destructuring root object declaration"
. Does that sound better? And I'd be personally ok having enableRootFixer
default to false
, but sometimes I think it is good when the defaults are--within reason and not genuinely harmful--a little on the aggressive side since it informs users of the features they might not learn about otherwise, and if they don't want it, they can come looking for how to disable.
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.
Missing JSDoc @param declaration for destructured parameter {bar}
.
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.
That could potentially become quite long though, no? e.g., {bar, baz, foo: {def: {ghi}}, many, others}
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 guess we could just slice it to a certain maximum length, adding ellipses as needed, and the curly bracket always at the end.
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.
How should we differentiate in this test case though?
code: `
/**
*
*/
function quux ({foo}) {
}
`,
errors: [
{
message: 'Missing JSDoc @param "root0" declaration.',
},
{
message: 'Missing JSDoc @param "root0.foo" declaration.',
},
],
output: `
/**
* @param root0
* @param root0.foo
*/
function quux ({foo}) {
}
`,
Report Missing JSDoc @param "{foo}" declaration
twice?
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.
We don't want to report just once because the root object may or may not already be defined, and if not defined, there should continue to be a separate message and fix.
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.
Btw, if it's ok, can we discuss this as a separate issue for fixing reporting messages? This discussion, though valid, is concerning a preexisting behavior (we refer to root0
in other messages, not just in this PR). It would be nice to get this merged so the disabling functionality is available sooner.
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.
88ebe41
to
01280b4
Compare
checkDestructured
property to disable destructured checks
01280b4
to
920f2e1
Compare
e7252db
to
c251641
Compare
c251641
to
bc5614b
Compare
🎉 This PR is included in version 25.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Currently builds on #531
Allows disabling of destructured checks (by setting
checkDestructured
tofalse
).Fixes #530.