Update: allowFunctionParams option in no-underscore-dangle (fixes 12579)

[sunghyunjo]

Prerequisites checklist

• I have read the contributing guidelines.

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

[jsf-clabot]

All committers have signed the CLA.

[yeonjuan]

@sunghyunjo
Thanks for working on this 👍 . I left some comments

[sunghyunjo]

@sunghyunjo
Thanks for working on this 👍 . I left some comments

@yeonjuan
Thanks for the review. Everything that was reviewed has been corrected.

[kaicataldo]

Thanks for working on this!

[sunghyunjo]

Thanks for working on this!

@kaicataldo
Corrected what you mentioned. Please tell me if there is any part I misunderstood.
Thanks for the review. 👍

[yeonjuan]

Thanks for the change 👍 I left some reviews.

[kaicataldo]

LGTM. Thanks, this looks great!

[mdjermanovic]

Should we also check this parameter: (_a = 0) => {}

[mdjermanovic]

Also not sure about this: (..._a) => {}

It might also make sense to check some identifiers in destructuring, like ([_a]) => {}, but since we're not checking those identifiers in variable declarations, then we probably shouldn't check them in parameters.

[mdjermanovic]

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?

[yeonjuan]

@mdjermanovic

Not checking function expression names might have been an oversight, but checking them now could introduce some conflicts:

I agree. 👍 I think this rule should check function expression names also.

Maybe we shouldn't change this behavior now?

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 adding allowFunctionParams option. :)

[mdjermanovic]

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 adding allowFunctionParams option. :)

I support this, we can add node.type === "FunctionDeclaration" condition to keep the current behavior when checking node.id.

@kaicataldo thoughts?

[kaicataldo]

Good catch! We definitely want to avoid situations where rules conflict with each other without some kind of escape hatch.

I support this, we can add node.type === "FunctionDeclaration" condition to keep the current behavior when checking node.id.

This seems like a good solution to me 👍

[sunghyunjo]

Should we also check this parameter: (_a = 0) => {}

@mdjermanovic
I have a question! I added a test case for this,
I couldn't pass the invalid case.

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
I think this is because the current code does not check the AssignmentPattern such as _bar = 0.

function printTips(_bar = 0, _let) {
  //
}

Using the code above as an example,
When AST is extracted from AST Explorer, it is determined that the AssignmentPattern needs additional logic as shown below.

Please answer if you want additional implementation or if my opinion is wrong.

[mdjermanovic]

I think that with allowFunctionParams: false this rule should report _bar = 0 and ..._bar.

So, if a param is AssignmentPattern, then the rule should check its left (if it is an Identifier). If a param is RestElement, then the rule should check its argument (if it is an Identifier).

Given how the rule already works with variable declarations, I think it shouldn't go further into destructuring patterns.

@kaicataldo, @yeonjuan thoughts?

[yeonjuan]

I think that with allowFunctionParams: false this rule should report _bar = 0 and ..._bar.

I think so. :). I think the option should check both(AssignmentPattern, RestElement).

Given how the rule already works with variable declarations, I think it shouldn't go further into destructuring patterns.

Agree 👍 Non-checking about the destructuring pattern seems a bug to me. But I think we can evaluate the issue separately.

[sunghyunjo]

@mdjermanovic @yeonjuan
Thank you for answer. One answer has not arrived, But I agree with you.
Added additional logic for RestElement and AssignmentPattern. 😃

[yeonjuan]

LGTM! Thanks 👍

[mdjermanovic]

Looks great, I left just a couple of notes about examples in the docs, and the tests.

[aladdin-add]

To make more sense, it should defaults to false. we can change it in next major release( as it's a breaking change). 😄

[sunghyunjo]

@mdjermanovic
Thanks for the review. I added all the test cases you mentioned.
Please make sure I understand and update it correctly.

[mdjermanovic]

Looks great, thanks!

[kaicataldo]

This LGTM. Thanks for contributing to ESLint!