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

Rule Change: Add onlyOneSimpleParam: boolean option to no-confusing-arrow #15548

Closed
1 task
JLHwung opened this issue Jan 27, 2022 · 6 comments · Fixed by #15566
Closed
1 task

Rule Change: Add onlyOneSimpleParam: boolean option to no-confusing-arrow #15548

JLHwung opened this issue Jan 27, 2022 · 6 comments · Fixed by #15566
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects

Comments

@JLHwung
Copy link
Contributor

JLHwung commented Jan 27, 2022

What rule do you want to change?

no-confusing-arrow

What change to do you want to make?

Generate fewer warnings

How do you think the change should be implemented?

A new option

Example code

() => 1 ? 2 : 3;
(a, b) => 1 ? 2 : 3;
({ a }) => 1 ? 2 : 3;
([ a ]) => 1 ? 2 : 3;
(...a) => 1 ? 2 : 3;

What does the rule currently do for this code?

Warn "Arrow function used ambiguously with a conditional expression."

What will the rule do after it's changed?

I propose a new onlyOneSimpleParam option with false default value.

When this option is set to true, the rule will not report any errors if one of the following is satisfied:

  1. There are more than 1 arguments or no arguments
  2. The argument is not an identifier

The intention of no-confusing-arrow is to check whether a BinaryExpression is incorrectly written as an arrow expression. When the arrow contains no argument, the expected BinaryExpression () >= 1 is never valid so we don't have to throw at all. Actually I think this behaviour should be enabled unconditionally.

When the arrow contains more than one argument: the expected BinaryExpression (a, b) >= 1 ? 2 : 3 is rare use case so we don't throw

When the arrow param is non-simple: the expected BinaryExpression ({ a, b }) >= 1 ? 2 : 3 is even rarer so we don't throw.

Note that all the current rule example code gives only an arrow expression with exactly one simple parameter. We could enable this option by default in future majors.

Participation

  • I am willing to submit a pull request to implement this change.

Additional comments

Context: babel/babel#14201 (comment)

@JLHwung JLHwung added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules labels Jan 27, 2022
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Jan 27, 2022
@nzakas
Copy link
Member

nzakas commented Jan 29, 2022

I think such an option would make sense.

@eslint/eslint-tsc thoughts?

@nzakas nzakas moved this from Needs Triage to Feedback Needed in Triage Jan 29, 2022
@btmills
Copy link
Member

btmills commented Jan 30, 2022

Just to clarify, the idea is that with this option, the rule would only report arrow functions with a single argument that is either an identifier or a primitive literal?

I’d be fine not warning for 0-argument arrows by default, either as part of this or a separate change.

@Gautam-Arora24
Copy link
Contributor

Gautam-Arora24 commented Jan 31, 2022

I would like to work on this issue :)

@JLHwung
Copy link
Contributor Author

JLHwung commented Jan 31, 2022

@Gautam-Arora24 Sure. I didn't have bandwidth yet. Please feel free to claim it.

@nzakas
Copy link
Member

nzakas commented Feb 1, 2022

Just to clarify, the idea is that with this option, the rule would only report arrow functions with a single argument that is either an identifier or a primitive literal?

That’s correct.

I’d be fine not warning for 0-argument arrows by default, either as part of this or a separate change.

For simplicity, I’d say let’s do it as part of the new option for now. We can tinker with defaults later if we think it’s necessary.

@btmills
Copy link
Member

btmills commented Feb 1, 2022

Makes sense to me. Since mdjermanovic left a 👍 on the original post, marking as accepted. You're good to go, @Gautam-Arora24!

@btmills btmills added the accepted There is consensus among the team that this change meets the criteria for inclusion label Feb 1, 2022
@btmills btmills moved this from Feedback Needed to Ready to Implement in Triage Feb 1, 2022
Triage automation moved this from Ready to Implement to Complete Feb 18, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Aug 18, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

Successfully merging a pull request may close this issue.

4 participants