Update: enforceForLogicalOperands no-extra-boolean-cast (fixes #12137) #12734

jmoore914 · 2020-01-02T01:57:06Z

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:

fixes #12137

What changes did you make? (Give an overview)

Changed lib/rules/no-extra-boolean-cast.js to allow an option for to enforce the rule in logical operands
Changed tests/lib/rules/no-extra-boolean-cast.js to add tests for the new option
Changed docs/rules/no-extra-boolean-cast.md to document the new option

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

(fixes #12137)

…ttps://github.com/jmoore914/eslint into no-extra-boolean-cast-enforce-for-logical-operands

mdjermanovic · 2020-01-02T02:30:31Z

@jmoore914 the title can be just:

Update: enforceForLogicalOperands no-extra-boolean-cast (fixes #12137)

jmoore914 · 2020-01-02T03:35:13Z

@mdjermanovic thanks, title has been updated!

mdjermanovic · 2020-01-02T03:48:11Z

Re-run the checks and it's passing now. It happens sometimes on macOS-latest for an unknown reason.

mdjermanovic

Thanks for the PR, this looks very good in general!

lib/rules/no-extra-boolean-cast.js

…wer parameters to functions; check precedence in fixer

jmoore914 · 2020-01-11T22:08:15Z

Nice suggestions! Let me know what you think now :)

lib/rules/no-extra-boolean-cast.js

tests/lib/rules/no-extra-boolean-cast.js

lib/rules/no-extra-boolean-cast.js

docs/rules/no-extra-boolean-cast.md

tests/lib/rules/no-extra-boolean-cast.js

docs/rules/no-extra-boolean-cast.md

lib/rules/no-extra-boolean-cast.js

…edence check; added additional example

…cast-enforce-for-logical-operands

mdjermanovic

Thanks for the changes! Looks great, just left several minor suggestions.

docs/rules/no-extra-boolean-cast.md

lib/rules/no-extra-boolean-cast.js

tests/lib/rules/no-extra-boolean-cast.js

…cast-enforce-for-logical-operands

docs/rules/no-extra-boolean-cast.md

tests/lib/rules/no-extra-boolean-cast.js

mdjermanovic

Thanks! Just a couple of minor suggestions and I think this is finished.

docs/rules/no-extra-boolean-cast.md

lib/rules/no-extra-boolean-cast.js

tests/lib/rules/no-extra-boolean-cast.js

…inserting space

kaicataldo

One small question about checking if Boolean is an actual global, but otherwise this LGTM!

kaicataldo · 2020-01-27T23:04:47Z

tests/lib/rules/no-extra-boolean-cast.js

@@ -1,6 +1,6 @@
 /**
- * @fileoverview Tests for no-extra-boolean-cast rule.
- * @author Brandon Mills
+ *@fileoverview Tests for no-extra-boolean-cast rule.


Mind reverting these two line changes? Seems like an accidental change.

kaicataldo · 2020-01-27T23:11:12Z

lib/rules/no-extra-boolean-cast.js

+        function isBooleanFunctionOrConstructorCall(node) {
+
+            // Boolean(<bool>) and new Boolean(<bool>)
+            return (node.type === "CallExpression" || node.type === "NewExpression") &&


Do we want to ensure that Boolean isn't a shadowed global variable?

That should be also done in CallExpression(node).

Though I think that both global Boolean fixes can be a separate PR because it's an existing issue (this function was extracted from UnaryExpression(node)).

Separating it out into a separate PR sounds good to me!

mdjermanovic

Looks great, thanks!

This PR also fixes 3 bugs in the default behavior:

/*eslint no-extra-boolean-cast: ["error"]*/

// these were false negatives
Boolean(Boolean(a));
new Boolean(Boolean(a));

// these were false positives
Boolean(a, !!b);
new Boolean(a, !!b);

// this was invalid autofix to: a = b ? c : d;
!!(a = b) ? c : d;

Online Demo

Side note: the fixer can be improved to avoid some unnecessary parens. This can be a separate PR as it's already happening in the actual master version for extra Boolean() so I believe it isn't directly related to this enhancement.

Also, it's questionable should the fixer add parens around b || c for if (a || Boolean(b || c)) {} since a || b || c has different semantics. I think this isn't a big issue as it doesn't change behavior, it can be evaluated and fixed if necessary in the same PR for the extra parens.

mdjermanovic · 2020-02-09T21:10:06Z

I edited the description just to link the issue. Not sure why that issue doesn't appear under "Linked issues" in the list of issues to choose.

@jmoore914 I think that the accidental change noticed in this comment is the only unaddressed concern at the moment.

…cast-enforce-for-logical-operands

jmoore914 · 2020-02-09T21:18:22Z

Could've sworn I fixed that! Well, should be all good now

mdjermanovic

LGTM, thanks!

kaicataldo · 2020-02-14T23:36:11Z

Thanks for contributing!

Standard8 · 2020-02-15T09:05:54Z

@jmoore914 Thank you for taking this on and getting it landed. I look forward to being able to use it soon.

…#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

jmoore914 added 3 commits January 1, 2020 17:37

Working rule, updated documentation

dc05d56

New: Added enforceForLogicalOperands option to no-extra-boolean-cast

288d20e

(fixes #12137)

Merge branch 'no-extra-boolean-cast-enforce-for-logical-operands' of h…

3bf2443

…ttps://github.com/jmoore914/eslint into no-extra-boolean-cast-enforce-for-logical-operands

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jan 2, 2020

jmoore914 changed the title ~~no-extra-boolean-cast option to enforce for logical operands (#12137)~~ New no-extra-boolean-cast option to enforce for logical operands (#12137) Jan 2, 2020

jmoore914 closed this Jan 2, 2020

jmoore914 reopened this Jan 2, 2020

jmoore914 changed the title ~~New no-extra-boolean-cast option to enforce for logical operands (#12137)~~ New: Added enforceForLogicalOperands option to no-extra-boolean-cast (fixes #12137) Jan 2, 2020

jmoore914 closed this Jan 2, 2020

jmoore914 reopened this Jan 2, 2020

jmoore914 changed the title ~~New: Added enforceForLogicalOperands option to no-extra-boolean-cast (fixes #12137)~~ Update: enforceForLogicalOperands no-extra-boolean-cast (fixes #12137) Jan 2, 2020

mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Jan 2, 2020

mdjermanovic requested changes Jan 11, 2020

View reviewed changes

Add default for schema option; renamed functions for clarity; pass fe…

9d50149

…wer parameters to functions; check precedence in fixer

mdjermanovic requested changes Jan 13, 2020

View reviewed changes

jmoore914 added 2 commits January 17, 2020 16:23

Removed extra variable from function; fixed function name; fixed prec…

00639b4

…edence check; added additional example

Merge remote-tracking branch 'upstream/master' into no-extra-boolean-…

96068d7

…cast-enforce-for-logical-operands

mdjermanovic requested changes Jan 24, 2020

View reviewed changes

jmoore914 added 5 commits January 25, 2020 08:41

Updated documentation

f086905

Removed outdated legacy code method

917217c

Added additional tests

a0926cc

Renamed function

47d82db

Merge remote-tracking branch 'upstream/master' into no-extra-boolean-…

42d7b30

…cast-enforce-for-logical-operands

mdjermanovic requested changes Jan 26, 2020

View reviewed changes

docs/rules/no-extra-boolean-cast.md Outdated Show resolved Hide resolved

docs/rules/no-extra-boolean-cast.md Outdated Show resolved Hide resolved

tests/lib/rules/no-extra-boolean-cast.js Show resolved Hide resolved

Added additional test; updated documentation

a344831

mdjermanovic requested changes Jan 26, 2020

View reviewed changes

Switched docs examples order; fixed comments in rule; added test for …

1af8aac

…inserting space

kaicataldo reviewed Jan 27, 2020

View reviewed changes

mdjermanovic approved these changes Jan 27, 2020

View reviewed changes

jmoore914 added 2 commits February 9, 2020 16:15

Merge remote-tracking branch 'upstream/master' into no-extra-boolean-…

2388278

…cast-enforce-for-logical-operands

Fixed accidentally edited lines

fd1ed90

mdjermanovic approved these changes Feb 9, 2020

View reviewed changes

kaicataldo approved these changes Feb 14, 2020

View reviewed changes

kaicataldo merged commit 41de9df into eslint:master Feb 14, 2020

jmoore914 deleted the no-extra-boolean-cast-enforce-for-logical-operands branch February 15, 2020 12:47

eslint-deprecated bot locked and limited conversation to collaborators Aug 14, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: enforceForLogicalOperands no-extra-boolean-cast (fixes #12137) #12734

Update: enforceForLogicalOperands no-extra-boolean-cast (fixes #12137) #12734

jmoore914 commented Jan 2, 2020 •

edited by mdjermanovic

mdjermanovic commented Jan 2, 2020

jmoore914 commented Jan 2, 2020 •

edited

mdjermanovic commented Jan 2, 2020

mdjermanovic left a comment

jmoore914 commented Jan 11, 2020

mdjermanovic left a comment

mdjermanovic left a comment

kaicataldo left a comment

kaicataldo Jan 27, 2020

kaicataldo Jan 27, 2020

mdjermanovic Jan 27, 2020

kaicataldo Jan 27, 2020

mdjermanovic left a comment

mdjermanovic commented Feb 9, 2020

jmoore914 commented Feb 9, 2020

mdjermanovic left a comment

kaicataldo commented Feb 14, 2020

Standard8 commented Feb 15, 2020

Update: enforceForLogicalOperands no-extra-boolean-cast (fixes #12137) #12734

Update: enforceForLogicalOperands no-extra-boolean-cast (fixes #12137) #12734

Conversation

jmoore914 commented Jan 2, 2020 • edited by mdjermanovic

mdjermanovic commented Jan 2, 2020

jmoore914 commented Jan 2, 2020 • edited

mdjermanovic commented Jan 2, 2020

mdjermanovic left a comment

Choose a reason for hiding this comment

jmoore914 commented Jan 11, 2020

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

kaicataldo Jan 27, 2020

Choose a reason for hiding this comment

kaicataldo Jan 27, 2020

Choose a reason for hiding this comment

mdjermanovic Jan 27, 2020

Choose a reason for hiding this comment

kaicataldo Jan 27, 2020

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic commented Feb 9, 2020

jmoore914 commented Feb 9, 2020

mdjermanovic left a comment

Choose a reason for hiding this comment

kaicataldo commented Feb 14, 2020

Standard8 commented Feb 15, 2020

jmoore914 commented Jan 2, 2020 •

edited by mdjermanovic

jmoore914 commented Jan 2, 2020 •

edited