feat(eslint-plugin): add no-unnecessary-boolean-literal-compare #242

JoshuaKGoldberg · 2019-02-10T22:05:00Z

Adds the equivalent of TSLint's boolean-literal-compare rule.

~~Does not add the auto-fixer; I ran out of time this weekend to learn how ESLint's auto-fixing works. 😉 Hoping to get to that eventually - either in this PR or a subsequent one, per your preference.~~ Added!

bradzacher · 2019-02-11T02:54:34Z

Hey @JoshuaKGoldberg, sorry for the pain, but we just merged a big PR to convert the plugin to typescript.
Please have a look through the source code and migrate your PR to typescript as well.
Let me know if you've got any questions!

packages/eslint-plugin/tests/lib/rules/boolean-literal-compare.js

mysticatea

Thank you for your contribution!

My first impression was "is this TypeScript-specific rule?". However, indeed, it needs the type of the other side operand to check whether the removal of === true doesn't change the expression result.

Would you improve tests a bit?

j-f1 · 2019-02-11T18:13:51Z

packages/eslint-plugin/ROADMAP.md

@@ -148,7 +148,7 @@
 | [`newline-per-chained-call`]        | 🌟  | [`newline-per-chained-call`][newline-per-chained-call]                              |
 | [`new-parens`]                      | 🌟  | [`new-parens`][new-parens]                                                          |
 | [`no-angle-bracket-type-assertion`] | ✅  | [`@typescript-eslint/no-angle-bracket-type-assertion`]                              |
-| [`no-boolean-literal-compare`]      | 🛑  | N/A                                                                                 |
+| [`no-boolean-literal-compare`]      | 🌓  | [`@typescript-eslint/boolean-literal-compare`]                                      |


Don’t forget to add the reference for this at the bottom so the link works.

packages/eslint-plugin/lib/rules/boolean-literal-compare.js

JamesHenry · 2019-04-03T16:53:52Z

@JoshuaKGoldberg Was there a discussion around dropping the no- for the ESLint version?

Personal and subjective, but I feel like the original name might be more intuitive...

JoshuaKGoldberg · 2019-04-03T22:13:36Z

@JamesHenry #203 started a discussion but it hasn't been resolved.

JamesHenry · 2019-04-04T14:58:23Z

Ok, let’s stick with the established ESLint conventions and start with no-

I do not foresee this rule having opposite behaviour.

Worst case scenario, we have a clear approach outlined in that issue for how to transition rules that change over time.

As soon as the prefix is added we’ll merge this, thanks so much again for your patience coming in just before the TS rewrite!

Stale

merlinnot · 2019-04-04T15:08:58Z

I'd actually see some room for the opposite rule, that's exactly what we would like to use in my company. It makes it more clear, especially that in TS you use exclamation marks for a different purpose (just an opinion, but that's why we have options, so people can choose).

JamesHenry · 2019-04-04T15:27:44Z

Hmm ok, I guess I can understand why some users would want that!

I think we have two options:

1. Merge the rule as it is currently in this PR as no-boolean-literal-compare
1. Keep the name, but add the option and missing functionality

I don't think we should get in the habit of merging rules based on the idea that "someday" options might come etc. It will just lead to inconsistencies and confusion.

@merlinnot If you are championing the other option, would you be available to help @JoshuaKGoldberg add those pieces?

merlinnot · 2019-04-04T15:31:06Z

I can work on the second option. That would be my firs contribution to this repository, but I'll give it a shot this evening. Shall I make a PR against this branch?

JamesHenry · 2019-04-04T15:32:20Z

Thanks a lot! @JoshuaKGoldberg are you happy with that plan?

JoshuaKGoldberg · 2019-04-04T15:36:07Z

Yeah let's do it! 🙌

One additional point against no-boolean-literal-compare is that the existing rule doesn't actually ban comparing against boolean literals. It just bans comparing nodes/results of type boolean from being compared against them. A more accurate name prefixed with no- would be something like no-unnecessary-boolean-literal-compare. +1 to the simpler name of boolean-literal-compare with the better features.

merlinnot · 2019-04-04T17:02:52Z

Ok, so I cloned a repo and now I'm thinking what to implement exactly. I thought it might be better to discuss it first.

My proposal:
I guess we can all agree that comparing booleans to true doesn't make any sense. So the only issue is how to deal with negation: allow comparison against false or prefix with an exclamation mark (!). That's why I'd call the option negationType and allow three values:

logical-not-operator
explicit-comparison
any

WDYT? What should be a default value?

merlinnot · 2019-04-10T13:48:14Z

Hey guys, any thoughts on this? I'm not sure who can make a call here :)

JoshuaKGoldberg · 2019-04-13T00:07:41Z

I think that would be @JamesHenry 😉

My two cents: it feels like the spirit of the rule is intended to standardize on how you compare booleans. If the default is to ban === true, it would feel weird to either allow === false or prefer it altogether. IMO the default configuration for negationType should be to ban === false.

I'm also not totally sold on negationType as an option name, as Type doesn't match up with what's being switched on. Maybe againstTrue / againstFalse?

JoshuaKGoldberg · 2020-01-25T15:43:22Z

Ok is there anything that needs to be done here? What would you like me to do, if anything? 😄

bradzacher

Okay revisiting and reviewing everything here.

I don't think there's any need for the reverse of this rule. We can consider adding that functionality if someone asks for it, but I don't think it's worth adding the extra code if we don't think anyone will use it.

So what needs to be done to get this merged?

Rebase on master
- there have been significant changes to the codebase since april 2019, so things like metadata and docs are out of date.
- there will probably be a lot of lint errors that need to be fixed up
Name the rule no-unnecessary-boolean-literal-compare.
- I don't have a problem with verbose names. I think a verbose name is better because it clearly communicates what the rule does.
- we have long names like no-unnecessary-type-assertion and no-non-null-asserted-optional-chain, so the length doesn't matter
- As mentioned - I'm okay with the no- prefix, because we don't have any drive or use-cases for the alternative.
address https://github.com/typescript-eslint/typescript-eslint/pull/242/files#r255630299
address my comments

Review notes:

don't forget to add it to src/rules/index.ts
don't forget to regen the configs
you can check docs and configs via yarn check:docs and yarn check:configs

Once the above is done, I'll do a second pass and it should then be good to go.

packages/eslint-plugin/docs/rules/boolean-literal-compare.md

packages/eslint-plugin/src/rules/boolean-literal-compare.ts

bradzacher

LGTM. thanks for coming back to this.

stale

BREAKING CHANGE: new rule @typescript-eslint/no-unnecessary-boolean-literal-compare typescript-eslint/typescript-eslint#242 BREAKING CHANGE: new rule @typescript-eslint/no-dupe-class-members https://github.com/typescript-eslint/typescript-eslint/blob/v2.19.0/packages/eslint-plugin/docs/rules/no-dupe-class-members.md BREAKING CHANGE: new rule @typescript-eslint/switch-exhaustiveness-check https://github.com/typescript-eslint/typescript-eslint/blob/v2.19.0/packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.md Closes #223.

mysticatea reviewed Feb 11, 2019

View reviewed changes

packages/eslint-plugin/tests/lib/rules/boolean-literal-compare.js Outdated Show resolved Hide resolved

mysticatea reviewed Feb 11, 2019

View reviewed changes

packages/eslint-plugin/tests/lib/rules/boolean-literal-compare.js Outdated Show resolved Hide resolved

mysticatea reviewed Feb 11, 2019

View reviewed changes

packages/eslint-plugin/tests/lib/rules/boolean-literal-compare.js Outdated Show resolved Hide resolved

mysticatea previously requested changes Feb 11, 2019

View reviewed changes

bradzacher added BLOCKED: awaiting ts migration enhancement: new plugin rule New rule request for eslint-plugin and removed enhancement: new plugin rule New rule request for eslint-plugin labels Feb 11, 2019

j-f1 previously requested changes Feb 11, 2019

View reviewed changes

JamesHenry previously requested changes Feb 23, 2019

View reviewed changes

packages/eslint-plugin/lib/rules/boolean-literal-compare.js Outdated Show resolved Hide resolved

JamesHenry removed the BLOCKED: awaiting ts migration label Apr 4, 2019

JamesHenry added the awaiting response Issues waiting for a reply from the OP or another party label Apr 4, 2019

bradzacher added the help wanted Extra attention is needed label Nov 22, 2019

bradzacher requested changes Jan 26, 2020

View reviewed changes

bradzacher removed the help wanted Extra attention is needed label Jan 26, 2020

JoshuaKGoldberg changed the title ~~feat(eslint-plugin): added new rule boolean-literal-compare~~ feat(eslint-plugin): added new rule no-unnecessary-boolean-literal-compare Jan 26, 2020

feat(eslint-plugin): added new rule boolean-literal-compare

853af0d

JoshuaKGoldberg force-pushed the typescript-eslint-boolean-literal-compare branch from fd20323 to 853af0d Compare January 27, 2020 04:17

JoshuaKGoldberg added 3 commits January 26, 2020 23:23

Merge branch 'master' (again)

2a1c5f8

Fixed up docs tests

ee9782e

Merge branch 'master'

12441e6

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 27, 2020

bradzacher approved these changes Jan 29, 2020

View reviewed changes

Merge branch 'master' into typescript-eslint-boolean-literal-compare

3d06de1

bradzacher changed the title ~~feat(eslint-plugin): added new rule no-unnecessary-boolean-literal-compare~~ feat(eslint-plugin): add no-unnecessary-boolean-literal-compare Jan 29, 2020

bradzacher merged commit 6bebb1d into typescript-eslint:master Jan 29, 2020

JoshuaKGoldberg deleted the typescript-eslint-boolean-literal-compare branch January 29, 2020 18:53

github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): add no-unnecessary-boolean-literal-compare #242

feat(eslint-plugin): add no-unnecessary-boolean-literal-compare #242

JoshuaKGoldberg commented Feb 10, 2019 •

edited

Loading

bradzacher commented Feb 11, 2019

mysticatea left a comment

j-f1 Feb 11, 2019

JamesHenry commented Apr 3, 2019

JoshuaKGoldberg commented Apr 3, 2019

JamesHenry commented Apr 4, 2019

merlinnot commented Apr 4, 2019

JamesHenry commented Apr 4, 2019

merlinnot commented Apr 4, 2019

JamesHenry commented Apr 4, 2019

JoshuaKGoldberg commented Apr 4, 2019

merlinnot commented Apr 4, 2019

merlinnot commented Apr 10, 2019

JoshuaKGoldberg commented Apr 13, 2019

JoshuaKGoldberg commented Jan 25, 2020

bradzacher left a comment

bradzacher left a comment

feat(eslint-plugin): add no-unnecessary-boolean-literal-compare #242

feat(eslint-plugin): add no-unnecessary-boolean-literal-compare #242

Conversation

JoshuaKGoldberg commented Feb 10, 2019 • edited Loading

bradzacher commented Feb 11, 2019

mysticatea left a comment

Choose a reason for hiding this comment

j-f1 Feb 11, 2019

Choose a reason for hiding this comment

JamesHenry commented Apr 3, 2019

JoshuaKGoldberg commented Apr 3, 2019

JamesHenry commented Apr 4, 2019

merlinnot commented Apr 4, 2019

JamesHenry commented Apr 4, 2019

merlinnot commented Apr 4, 2019

JamesHenry commented Apr 4, 2019

JoshuaKGoldberg commented Apr 4, 2019

merlinnot commented Apr 4, 2019

merlinnot commented Apr 10, 2019

JoshuaKGoldberg commented Apr 13, 2019

JoshuaKGoldberg commented Jan 25, 2020

bradzacher left a comment

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Feb 10, 2019 •

edited

Loading