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

Update: Add never option for new-parens (refs #10034) #11379

Merged
merged 4 commits into from May 25, 2019

Conversation

pfgithub
Copy link
Contributor

@pfgithub pfgithub commented Feb 12, 2019

What is the purpose of this pull request? (put an "X" next to item)

[ ] 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:

#10034 was closed automatically for a lack of interest, I'm not sure if this functionality is wanted.

What changes did you make? (Give an overview)

Added an argument to new-parens for always or never. The default with no arguments keeps the same functionality. Added tests for the never option and updated documentation.

Is there anything you'd like reviewers to focus on?

The fixer will always put parenthesis even when they're not required.

What rule do you want to change?

new-parens

Does this change cause the rule to produce more or fewer warnings?

No

How will the change be implemented? (New option, new default behavior, etc.)?

Adds an option for enforcing or disallowing parenthesis on constructors with no arguments using new. Default behavior stays the same.

Please provide some example code that this change will affect:

Examples of incorrect code for this rule with the "never" option:

/*eslint new-parens: ["error", "never"]*/

var person = new Person();
var person = new (Person)();

Examples of correct code for this rule with the "never" option:

/*eslint new-parens: ["error", "never"]*/

var person = new Person;
var person = (new Person);
var person = new Person("Name");

What does the rule currently do for this code?

Errors because the arguments are invalid.

What will the rule do after it's changed?

The argument will no longer be invalid.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Feb 12, 2019
@aladdin-add aladdin-add added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules 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 Feb 12, 2019
@ilyavolodin
Copy link
Member

@eslint/eslint-team We completely lost track of this PR, does anyone else wants to support it/champion it?

@platinumazure
Copy link
Member

I'll champion. Just needs one more 👍 from the team.

@platinumazure platinumazure self-assigned this Mar 30, 2019
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have some tests explicitly using the "always" option (to make sure they behave the same as no option).

Also left a few other suggestions.

lib/rules/new-parens.js Outdated Show resolved Hide resolved
lib/rules/new-parens.js Outdated Show resolved Hide resolved
@pfgithub
Copy link
Contributor Author

pfgithub commented Mar 30, 2019

I added a few tests for explicitly using "always" and I also added some to make sure that "never" allows parenthesis when there are arguments.

I also fixed the comment and changed the message id and message.

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ESLint team still needs to accept this before merging, but the implementation looks good to me. Thanks for contributing!

@ilyavolodin
Copy link
Member

@eslint/eslint-team Need just one more +1 vote for this. It's time to either accept it, or close this issue.

@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 May 25, 2019
@kaicataldo kaicataldo merged commit cf9cce8 into eslint:master May 25, 2019
@kaicataldo
Copy link
Member

Thanks for contributing to ESLint!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Nov 22, 2019
@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 Nov 22, 2019
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants