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

Jump to bottom

[no-extra-parens] Allow an exception for (new X()).y #12428

Closed
domenic opened this issue Oct 14, 2019 · 8 comments · Fixed by WealthWizardsEngineering/service-registry#12, basics/vector#111, thinkwee/thinkwee.github.io#39, alxtford/numconv#46 or WoTTsecurity/api#773
Closed

[no-extra-parens] Allow an exception for (new X()).y #12428

domenic opened this issue Oct 14, 2019 · 8 comments · Fixed by WealthWizardsEngineering/service-registry#12, basics/vector#111, thinkwee/thinkwee.github.io#39, alxtford/numconv#46 or WoTTsecurity/api#773
Assignees
@mdjermanovic
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

@domenic
Copy link

domenic commented Oct 14, 2019

What rule do you want to change? no-extra-parens

Does this change cause the rule to produce more or fewer warnings? Fewer, if configured

How will the change be implemented? (New option, new default behavior, etc.)? New option, similar to the existing ones like returnAssign, nestedBinaryExpressions, etc.

Please provide some example code that this change will affect:

const doc = (new JSDOM()).window.document;

const { document } = (new JSDOM(`<!doctype html><html><head></head><body>
  <div></div><div title=""></div><div title="yes"></div>
</body></html>`)).window;

What does the rule currently do for this code?

It requires removing the parens, i.e. that I write

const doc = new JSDOM().window.document;

const { document } = new JSDOM(`<!doctype html><html><head></head><body>
  <div></div><div title=""></div><div title="yes"></div>
</body></html>`).window;

This is equivalent, but it feels really weird. Although the new operator binds more closely than the . operator in JS, this is a somewhat-surprising feature of JS, as . otherwise binds very high, and most space-separated operators bind quite low.

What will the rule do after it's changed?

Allow the above code, when a new option is configured, e.g."newExpressions": false.

Are you willing to submit a pull request to implement this change?

Yes, although a community volunteer would be much appreciated.

Other note

This changed in v6.5.0 as part of #12302.
@domenic domenic added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Oct 14, 2019
@nzakas nzakas added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 14, 2019
@nzakas
Copy link
Member

nzakas commented Oct 14, 2019

Thanks for reaching out. It does seem like we should have added an option for backwards compatibility when this rule was changed.

@mdjermanovic
Copy link
Member

I'm willing to champion and work on this.

@mdjermanovic mdjermanovic self-assigned this Oct 14, 2019
@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 14, 2019
@kaicataldo
Copy link
Member

This has now been accepted.

@mdjermanovic
Copy link
Member

Just to confirm the details of this enhancement, if I understand correctly the following should be done.

The option applies only to the change made in v6.5.0 (part of #12302):

(new X()).y;

(new X())[y];

Other cases of unnecessary parens around new X() that were warnings in previous versions will remain warnings, regardless of how the option is set. For example:

y = (new X());

f((new X()), y);

(new X())();

new (new X())();

The name of the option could be enforceForNewInMemberExpressions ?

Since it's default behavior in the actual version, the default value is true:

/* eslint no-extra-parens: "error" */

(new X()).y; // error

(new X())[y]; // error

y = (new X()); // error

f((new X()), y); // error

(new X())(); // error

new (new X())(); // error

When the option is explicitly set to false, (new X()).y and (new X())[y] are allowed:

/* eslint no-extra-parens: ["error", "all", { "enforceForNewInMemberExpressions": false }] */

(new X()).y; // ok

(new X())[y]; // ok

y = (new X()); // error

f((new X()), y); // error

(new X())(); // error

new (new X())(); // error

@mysticatea mysticatea added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed accepted There is consensus among the team that this change meets the criteria for inclusion labels Oct 15, 2019
@mysticatea
Copy link
Member

mysticatea commented Oct 15, 2019 •
edited

I have removed accepted label because there are three upvotes but there is a downvote from the team. This enhancement is a mixed reception and needs consensus.

My stance is neutral. It's hard for me to agree with the motivation to add the option because recent languages have the same precedence. But I know C++ is different.

About backward compatibility, I don't think it's a problem. This was a bug fix for the bug that no-extra-parens overlooked extra parens and our policy is that bug fixes that increase errors happens in minor releases.

@platinumazure
Copy link
Member

@mysticatea Michael is no longer on the team (see README). So his downvote is a community opinion, no more.

Would you like to 👎 this issue?

@mysticatea
Copy link
Member

mysticatea commented Oct 15, 2019 •
edited

Ah, Alumni isn't counted as the team's votes? In that case, I'm OK with accepted (I'm neutral on this issue).

@mysticatea mysticatea added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 15, 2019
@domenic
Copy link
Author

domenic commented Oct 15, 2019

Just to confirm the details of this enhancement, if I understand correctly the following should be done.

This makes sense to me. I think it's a reasonable question what the default should be (and thus what the name would be) but I'll leave this to the ESLint team.

mdjermanovic added a commit that referenced this issue Oct 15, 2019
@mdjermanovic
Update: enforceForNewInMemberExpressions no-extra-parens (fixes #12428)
0ffaba2
@mdjermanovic mdjermanovic mentioned this issue Oct 15, 2019
Merged
This was referenced Nov 9, 2019
Open
Open
@snyk-bot snyk-bot mentioned this issue Nov 21, 2019
Merged
@snyk-bot snyk-bot mentioned this issue Dec 1, 2019
Merged
@snyk-bot snyk-bot mentioned this issue Feb 13, 2020
Merged
@snyk-bot snyk-bot mentioned this issue Feb 27, 2020
Merged
@a-martynovich a-martynovich mentioned this issue Mar 10, 2020
Merged
This was referenced Mar 11, 2020
Merged
Open
This was referenced Mar 26, 2020
Merged
Merged
Merged
Open
This was referenced Apr 3, 2020
Merged
Merged
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Apr 19, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mdjermanovic mdjermanovic

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
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Snyk] Upgrade eslint from 6.3.0 to 6.6.0 WealthWizardsEngineering/service-registry
[Snyk] Upgrade eslint from 6.6.0 to 6.7.0 basics/vector
[Snyk] Upgrade eslint from 6.5.1 to 6.8.0 thinkwee/thinkwee.github.io
[Snyk] Upgrade eslint from 5.15.1 to 6.8.0 alxtford/numconv
[Snyk] Upgrade eslint from 6.0.0 to 6.8.0 WoTTsecurity/api
[Snyk] Upgrade eslint from 6.0.0 to 6.8.0 WealthWizardsEngineering/instance-terminator
[Snyk] Upgrade eslint from 6.6.0 to 6.8.0 nodeshift-archived/rhoaster
[Snyk] Upgrade eslint from 6.5.0 to 6.8.0 nodeshift/openshift-rest-client
[Snyk] Upgrade eslint from 6.0.0 to 6.8.0 jamesleesaunders/d3-interpolate-curve
[Snyk] Upgrade eslint from 5.4.0 to 6.8.0 LuisAlejandro/luisalejandro.github.io-legacy
6 participants
@nzakas @platinumazure @domenic @mysticatea @kaicataldo @mdjermanovic