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: no-empty-pattern allow parameters #16931

Closed
1 task
Faithfinder opened this issue Feb 25, 2023 · 10 comments · Fixed by #17365
Closed
1 task

Rule Change: no-empty-pattern allow parameters #16931

Faithfinder opened this issue Feb 25, 2023 · 10 comments · Fixed by #17365
Assignees
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

Comments

@Faithfinder
Copy link

What rule do you want to change?

no-empty-pattern

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

const handler = ({}, {}, usedParam) => {
  // use used param
}

What does the rule currently do for this code?

Warns

What will the rule do after it's changed?

I think this is a valid pattern as opposed to having _something naming scheme. I would love an option to allow this.

Participation

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

Additional comments

No response

@Faithfinder Faithfinder added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules labels Feb 25, 2023
@Rec0iL99
Copy link
Member

Hi @Faithfinder, thanks for the issue. This looks like a duplicate of #13315. The original issue could not get enough support from the team and hence was closed.

We currently aren't adding any new options or changes to the stylistic rules due to policy changes. You can always clone the existing rule and create a custom rule to fit your needs.

I'm not closing this for now since I'm not sure if this fits the description of a stylistic rule. Leaving it open for someone from the core team to look at.

@Faithfinder
Copy link
Author

Ah! Sorry I didn't do a good enough search. I don't think it's entirely stylistic though. I'll await the verdict then

@mdjermanovic
Copy link
Member

This rule is of type: "problem", so it is not considered stylistic.

I support this proposal 👍 though I'm not sure how common this pattern is as it doesn't work in case the argument can be nullish. Curious what other team members think about the proposal.

@snitin315
Copy link
Contributor

This proposal makes sense to me 👍🏻. Any suggestions for the option's name?

@Faithfinder
Copy link
Author

allowInParameters seems obvious?

@mdjermanovic
Copy link
Member

Perhaps something like allowObjectPatternsAsParameters?

I think the option should work like this:

({}) => {}; // ok

({} = {}) => {}; // also ok

({ foo: {} }) => {}; // not ok

([]) => {}; // not ok

"in" parameters might suggest that empty patterns anywhere inside parameters would be allowed, e.g., ({ foo: {} }) => {}.

"Object" because it only allows empty object patterns, not array patterns.

@snitin315
Copy link
Contributor

snitin315 commented Feb 27, 2023

👍🏻 for allowObjectPatternsAsParameters . Marking this as accepted.

@snitin315 snitin315 self-assigned this Feb 27, 2023
@snitin315 snitin315 added the accepted There is consensus among the team that this change meets the criteria for inclusion label Feb 27, 2023
@Faithfinder
Copy link
Author

I don't see why arrays shouldn't be allowed. Might be a natural choice in Typescript land, where the fact that it's an array is apparent

@Tanujkanti4441
Copy link
Contributor

Perhaps something like allowObjectPatternsAsParameters?

I think the option should work like this:

({}) => {}; // ok

({} = {}) => {}; // also ok

({ foo: {} }) => {}; // not ok

([]) => {}; // not ok

@mdjermanovic, as per above comment ({ foo: {} }) => {} // not ok will give an error when allowObjectPatternsAsParameters option will be true, even if it is there as a function parameter.

"in" parameters might suggest that empty patterns anywhere inside parameters would be allowed, e.g., ({ foo: {} }) => {}.

Here you said ({ foo: {} }) => {} whould be allowed inside anywhere in parameters
Would you please clear this part more.

And is this new option only should allow the 'ObjectPatterns' in Arrow function and not for the function declaration?

@mdjermanovic
Copy link
Member

as per above comment ({ foo: {} }) => {} // not ok will give an error when allowObjectPatternsAsParameters option will be true, even if it is there as a function parameter.

Yes, this should be an error. The empty pattern appears in parameters but it isn't in a parameter position so it doesn't make sense for the purpose of this option.

"in" parameters might suggest that empty patterns anywhere inside parameters would be allowed, e.g., ({ foo: {} }) => {}.

Here you said ({ foo: {} }) => {} whould be allowed inside anywhere in parameters Would you please clear this part more.

It should be allowed only at the first level, as the purpose is to skip positions in parameter lists.

And is this new option only should allow the 'ObjectPatterns' in Arrow function and not for the function declaration?

It should allow empty object pattern parameters in ArrowFunctionExpression, FunctionDeclaration and FunctionExpression nodes.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jan 18, 2024
@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 Jan 18, 2024
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
Development

Successfully merging a pull request may close this issue.

5 participants