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

Rule change: no-extra-boolean-cast should catch more cases #12137

Closed
Standard8 opened this issue Aug 21, 2019 · 9 comments · Fixed by #12734
Closed

Rule change: no-extra-boolean-cast should catch more cases #12137

Standard8 opened this issue Aug 21, 2019 · 9 comments · Fixed by #12734
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

@Standard8
Copy link
Contributor

What rule do you want to change?

no-extra-boolean-cast

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

More

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

New default behavior

Please provide some example code that this change will affect:

if (foo && !!bar) {}
if (!!foo && bar) {}

while (foo && bar && !!baz) {}

if (foo && Boolean(bar) {}

What does the rule currently do for this code?

No errors

What will the rule do after it's changed?

Throw a Redundant double negation error.

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

Possibly, depends what time I get.
@Standard8 Standard8 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 Aug 21, 2019
@mdjermanovic mdjermanovic 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 Aug 21, 2019
@mdjermanovic
Copy link
Member

👍 from me for the enhancement in general.

If accepted, this should probably be an option rather than default behavior, until the next major release.

The same could apply to || as well?

@g-plane g-plane 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 Aug 22, 2019
@g-plane
Copy link
Member

g-plane commented Aug 22, 2019

I prefer this is as default behavior personally.

@Standard8
Copy link
Contributor Author

I guess if it is default immediately or not, depends on if it is seen as breaking behaviour?

@platinumazure
Copy link
Member

platinumazure commented Aug 22, 2019 •
edited

As a reminder, we do need a team member to champion this rule proposal (assign to themselves) before we can mark this issue as accepted. Would anyone from the team like to champion this rule?

Regarding default behavior or not: If the proposal here is to change the default behavior, then this is a breaking change and a PR cannot be merged until the next major release. If we want the functionality available before then, we need to have this behavior behind an option with the default behavior unchanged.

@platinumazure platinumazure 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 Aug 22, 2019
@mdjermanovic
Copy link
Member

I'll champion this and there are already 3 other 👍, so marked as accepted!

This feature has to be behind an option at the moment. The option applies to both && and ||.

Any ideas for the name, maybe enforceForLogicalOperands?

@Standard8 would you like to submit a PR to implement this option?

@mdjermanovic mdjermanovic self-assigned this Sep 2, 2019
@mdjermanovic mdjermanovic 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 Sep 2, 2019
@Standard8
Copy link
Contributor Author

@Standard8 would you like to submit a PR to implement this option?

I'll be happy to, but realistically, it is going to be a couple of weeks before I get time to write it.

@mdjermanovic
Copy link
Member

I'll be happy to, but realistically, it is going to be a couple of weeks before I get time to write it.

Sounds good to me! It's perfectly fine whenever you get time.

@kaicataldo kaicataldo added help wanted The team would welcome a contribution from the community for this issue Hacktoberfest Recommended issue for those participating in Hacktoberfest https://hacktoberfest.digitalocean.com/ labels Sep 29, 2019
@kaicataldo kaicataldo removed the Hacktoberfest Recommended issue for those participating in Hacktoberfest https://hacktoberfest.digitalocean.com/ label Nov 19, 2019
@jmoore914
Copy link
Contributor

If @Standard8 doesn't have the time to work on this, I would be happy to!

@Standard8
Copy link
Contributor Author

@jmoore914 Please do, I'm still struggling to spare the time for this.

@jmoore914 jmoore914 mentioned this issue Jan 2, 2020
Merged
@mdjermanovic mdjermanovic removed the help wanted The team would welcome a contribution from the community for this issue label Jan 11, 2020
kaicataldo pushed a commit that referenced this issue Feb 14, 2020
@jmoore914
Update: enforceForLogicalOperands no-extra-boolean-cast (fixes #12137) (
41de9df 
#12734)

* Working rule, updated documentation

* New: Added enforceForLogicalOperands option to no-extra-boolean-cast
(fixes #12137)

* Add default for schema option; renamed functions for clarity; pass fewer parameters to functions; check precedence in fixer

* Removed extra variable from function; fixed function name; fixed precedence check; added additional example

* Updated documentation

* Removed outdated legacy code method

* Added additional tests

* Renamed function

* Added additional test; updated documentation

* Switched docs examples order; fixed comments in rule; added test for inserting space

* Fixed accidentally edited lines
montmanu pushed a commit to montmanu/eslint that referenced this issue Mar 4, 2020
@jmoore914 @montmanu
Update: enforceForLogicalOperands no-extra-boolean-cast (fixes eslint…
38a573d 
…#12137) (eslint#12734)

* Working rule, updated documentation

* New: Added enforceForLogicalOperands option to no-extra-boolean-cast
(fixes eslint#12137)

* Add default for schema option; renamed functions for clarity; pass fewer parameters to functions; check precedence in fixer

* Removed extra variable from function; fixed function name; fixed precedence check; added additional example

* Updated documentation

* Removed outdated legacy code method

* Added additional tests

* Renamed function

* Added additional test; updated documentation

* Switched docs examples order; fixed comments in rule; added test for inserting space

* Fixed accidentally edited lines
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Aug 14, 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 Aug 14, 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.

Update: enforceForLogicalOperands no-extra-boolean-cast (fixes #12137)
6 participants
@platinumazure @Standard8 @kaicataldo @g-plane @jmoore914 @mdjermanovic