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

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

Conversation

JoshuaKGoldberg
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg commented Feb 10, 2019

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
Copy link
Member

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!

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

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?

@bradzacher 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
j-f1 previously requested changes Feb 11, 2019
@@ -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`] |
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@JamesHenry
Copy link
Member

@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
Copy link
Member Author

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

@JamesHenry
Copy link
Member

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!

@merlinnot
Copy link

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
Copy link
Member

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?

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

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
Copy link
Member

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

@JoshuaKGoldberg
Copy link
Member Author

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
Copy link

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
Copy link

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

@JoshuaKGoldberg
Copy link
Member Author

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?

@bradzacher bradzacher added the help wanted Extra attention is needed label Nov 22, 2019
@JoshuaKGoldberg
Copy link
Member Author

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

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

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.

@bradzacher bradzacher removed the help wanted Extra attention is needed label Jan 26, 2020
@JoshuaKGoldberg 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
@JoshuaKGoldberg JoshuaKGoldberg force-pushed the typescript-eslint-boolean-literal-compare branch from fd20323 to 853af0d Compare January 27, 2020 04:17
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 27, 2020
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

LGTM. thanks for coming back to this.

@bradzacher 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 bradzacher merged commit 6bebb1d into typescript-eslint:master Jan 29, 2020
@JoshuaKGoldberg JoshuaKGoldberg deleted the typescript-eslint-boolean-literal-compare branch January 29, 2020 18:53
mightyiam added a commit to mightyiam/eslint-config-love that referenced this pull request Feb 4, 2020
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.
mightyiam added a commit to mightyiam/eslint-config-love that referenced this pull request Feb 11, 2020
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.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: new plugin rule New rule request for eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants