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
Update: allowFunctionParams option in no-underscore-dangle (fixes 12579) #13545
Conversation
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.
@sunghyunjo
Thanks for working on this 👍 . I left some comments
@yeonjuan |
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.
Thanks for working on this!
@kaicataldo |
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.
Thanks for the change 👍 I left some reviews.
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.
LGTM. Thanks, this looks great!
Should we also check this parameter: |
Also not sure about this: It might also make sense to check some identifiers in destructuring, like |
Not checking function expression names might have been an oversight, but checking them now could introduce some conflicts: /* eslint no-underscore-dangle: "error" */
/* eslint func-name-matching: "error" */
var foo = {
_x: function _x() {}
} /* eslint no-underscore-dangle: ["error", { "allowAfterThis": true }] */
/* eslint func-name-matching: "error" */
this._x = function _x() {}; Maybe we shouldn't change this behavior now? |
I agree. 👍 I think this rule should check
IMO, it seems good to keep current behavior(non-checking) in this PR, because I think checking function expression's name seems not deeply related with |
I support this, we can add @kaicataldo thoughts? |
Good catch! We definitely want to avoid situations where rules conflict with each other without some kind of escape hatch.
This seems like a good solution to me 👍 |
@mdjermanovic The invalid case is written as follows. { code: "function foo(_bar = 0) {}", options: [{ allowFunctionParams: false }], parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "unexpectedUnderscore", data: { identifier: "_bar" }, type: "Identifier" }] },
{ code: "const foo = { onClick(_bar = 0) { } }", options: [{ allowFunctionParams: false }], parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "unexpectedUnderscore", data: { identifier: "_bar" }, type: "Identifier" }] },
{ code: "const foo = (_bar = 0) => {}", options: [{ allowFunctionParams: false }], parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "unexpectedUnderscore", data: { identifier: "_bar" }, type: "Identifier" }] } IMO, The reason the invalid case is not passed function printTips(_bar = 0, _let) {
//
} Using the code above as an example, Please answer if you want additional implementation or if my opinion is wrong. |
I think that with So, if a Given how the rule already works with variable declarations, I think it shouldn't go further into destructuring patterns. @kaicataldo, @yeonjuan thoughts? |
I think so. :). I think the option should check both(AssignmentPattern, RestElement).
Agree 👍 Non-checking about the destructuring pattern seems a bug to me. But I think we can evaluate the issue separately. |
@mdjermanovic @yeonjuan |
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.
LGTM! Thanks 👍
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, I left just a couple of notes about examples in the docs, and the tests.
- `"allowAfterSuper": false` (default) disallows dangling underscores in members of the `super` object | ||
- `"allowAfterThisConstructor": false` (default) disallows dangling underscores in members of the `this.constructor` object | ||
- `"enforceInMethodNames": false` (default) allows dangling underscores in method names | ||
- `"allowFunctionParams": true` (default) allows dangling underscores in function parameter names |
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.
To make more sense, it should defaults to false
. we can change it in next major release( as it's a breaking change). 😄
@mdjermanovic |
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!
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 LGTM. Thanks for contributing to ESLint!
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[X] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
(fixes #12579 )
If there is an underscore at the beginning of the function parameter name, it will give a warning.
Is there anything you'd like reviewers to focus on?
None