feat(eslint-plugin): [prefer-nullish-coalescing] add `ignorePrimitives` option #6487

omril1 · 2023-02-18T09:13:56Z

PR Checklist

Addresses an existing open issue: fixes [prefer-nullish-coalescing] request for preferring "||" when string value #4906, fixes Bug: [prefer-nullish-coalescing] False positive to nullable boolean #7055
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Add ignorePrimitives option that can be set individually for string, boolean, number and bigint

typescript-eslint · 2023-02-18T09:13:59Z

Thanks for the PR, @omril1!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

nx-cloud · 2023-02-18T09:14:00Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 09f207a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 47 targets

Sent with 💌 from NxCloud.

netlify · 2023-02-18T09:14:05Z

❌ Deploy Preview for typescript-eslint failed.

Name	Link
🔨 Latest commit	`5a18ee3`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/64a90610640a7600088db0e9

codecov · 2023-02-18T09:28:30Z

Codecov Report

Merging #6487 (5a18ee3) into main (b1a23a9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6487   +/-   ##
=======================================
  Coverage   87.39%   87.40%           
=======================================
  Files         386      386           
  Lines       13198    13208   +10     
  Branches     3873     3878    +5     
=======================================
+ Hits        11535    11545   +10     
  Misses       1296     1296           
  Partials      367      367

Flag	Coverage Δ
unittest	`87.40% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...lint-plugin/src/rules/prefer-nullish-coalescing.ts	`100.00% <100.00%> (ø)`

JoshuaKGoldberg

A good start, thanks for sending this in! ❤️

Requesting changes on more test cases, docs cleanups, and some code shenanigans.

packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts

packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts

packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts

packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts

JoshuaKGoldberg · 2023-03-20T16:08:19Z

👋 Hey @omril1! Just checking in, is this still something you have time for? No worries if not - I just don't want to leave it hanging.

omril1 · 2023-03-20T18:02:16Z

@JoshuaKGoldberg I'm still on it, I have some more work that I left in the middle

xsjcTony · 2023-05-24T10:52:17Z

Any updates here? 😄

omril1 · 2023-05-24T11:02:55Z

Any updates here? 😄

Mmm, yeah I left it out for a while, got a bit of a burnout at work 🥲.
I'll get back to it maybe in the weekend.

…ullish-enhancment

nx-cloud · 2023-05-27T05:43:15Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 5a18ee3. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 25 targets

Sent with 💌 from NxCloud.

- Literal types (true, 1, etc.) - Template literals (`hello${string}`) - Bigints (1n) - Combinations of the types (boolean | string | ...), etc.

omril1 · 2023-06-16T02:45:41Z

The website tests keep failing in CI and I can't re-run them.
@JoshuaKGoldberg Is there anything I can do to move this forward?

JoshuaKGoldberg · 2023-06-24T17:32:31Z

Ah don't worry about the website CI tests, that's a separate thing. I'll put this back in the queue to review - thanks!

Outdated review

JoshuaKGoldberg · 2023-06-28T16:20:24Z

FWIW you don't have to keep merging main if you don't want. We're a little focused on v6 right now so the issue & PR review queue is taking longer than we'd normally want.

JoshuaKGoldberg

Code looks great! Just requesting changes on a docs example & clearer tests now. Thanks! 🚀

JoshuaKGoldberg · 2023-07-06T15:24:44Z

packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md

@@ -131,6 +131,15 @@ a ?? (b && c && d);

 **_NOTE:_** Errors for this specific case will be presented as suggestions (see below), instead of fixes. This is because it is not always safe to automatically convert `||` to `??` within a mixed logical expression, as we cannot tell the intended precedence of the operator. Note that by design, `??` requires parentheses when used with `&&` or `||` in the same expression.

+### `ignorePrimitives`


[Docs] Could you add some examples please? We generally try to -at least for newly added rule options- so users can see exactly how the code looks.

I tried to look at the style for examples with options, this rule seems unique in how the examples are presented but decided to keep it the same since I've noticed bradzacher did the same

Heh yeah we'll eventually clean all these up... eventually. Thanks!

JoshuaKGoldberg · 2023-07-06T15:25:34Z

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts

+      boolean?: boolean;
+      number?: boolean;
+      string?: boolean;
+    };


[Non-actionable] In theory we could allow this to be a boolean to configure all of the primitives at once. I think it's fine to leave that as a follow-up to this PR.

Good idea, leaving it for now.

Do I need to open an issue for it or can I reference the same one?

We should have a new issue for tracking. It gets confusing having multiple PRs target the same issue.

If you don't have the time to file a new issue, no worries (I do hate asking folks to fill out paperwork). I'll set a reminder for a couple days from now.

JoshuaKGoldberg · 2023-07-06T15:32:18Z

packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts

+        ignoreablePrimitive: ['boolean'],
+        literalPrimitive: 'true | false | null',
+      },
+    ].map<TSESLint.InvalidTestCase<MessageIds, Options>>(


.map

[Testing] I can't find a tracking right now, but I'm slowly getting more and more sour on generating tests this way. I'm finding it very difficult to read through them and the generation code obfuscates what the test actually does. Could you just have manually written error cases for these please?

I understand what you mean here, I'm all for flatting them, I thought it was how it works here based on a small sample of tests I've seen

Yeah :/ it's an unfortunate bit of IMO tech debt we should really clean up at some point...

…ullish-enhancment

JoshuaKGoldberg

💯 looks great, thanks for all the touchups! Sorry to keep you waiting 😅 this really is a great PR.

Omri Luzon added 2 commits February 18, 2023 11:09

issue-4906 - prefer-nullish-coalescing enhancment

b427e6b

remove comment

8e016d3

omril1 marked this pull request as draft February 18, 2023 09:14

add doc

09f207a

omril1 marked this pull request as ready for review February 18, 2023 09:26

JoshuaKGoldberg requested changes Feb 18, 2023

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Feb 18, 2023

Omri Luzon added 3 commits February 24, 2023 05:22

CR: sort keys

24fb9c5

CR: import once

7aec9fa

CR: remove unncessary "as const"

ed7d525

bradzacher added the enhancement: plugin rule option New rule option for an existing eslint-plugin rule label Apr 10, 2023

bradzacher mentioned this pull request May 24, 2023

Bug: [prefer-nullish-coalescing] False positive to nullable boolean #7055

Closed

4 tasks

Merge remote-tracking branch 'upstream/main' into issue-4906-prefer-n…

6954d62

…ullish-enhancment

Omri Luzon added 8 commits May 27, 2023 09:43

CR: Fix markdown lint rule

6be0360

CR: Use binary OR to simplify loop

a576095

CR: More positive test cases

8dd233f

- Literal types (true, 1, etc.) - Template literals (`hello${string}`) - Bigints (1n) - Combinations of the types (boolean | string | ...), etc.

CR: mixed

25a0852

CR: remove implied line

e66ee9b

CR: Move the note up

5c4ab38

prettier fixz

3b1e012

Fix coverage, ts didn't know it has a default so it's not optional

2e579a6

Fixed union test and JSON.stringify(ignorePrimitives)

2dff212

omril1 requested a review from JoshuaKGoldberg June 4, 2023 05:54

Omri Luzon added 3 commits June 6, 2023 13:08

Merge branch 'main' into issue-4906-prefer-nullish-enhancment

bea946b

Merge branch 'main' into issue-4906-prefer-nullish-enhancment

6d4aa01

Merge branch 'main' into issue-4906-prefer-nullish-enhancment

357fd59

Omri Luzon added 3 commits June 17, 2023 12:45

Merge branch 'main' into issue-4906-prefer-nullish-enhancment

d3f6ee0

Merge branch 'main' into issue-4906-prefer-nullish-enhancment

e34fb0a

Merge branch 'main' into issue-4906-prefer-nullish-enhancment

419a383

JoshuaKGoldberg removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 24, 2023

Omri Luzon added 3 commits June 26, 2023 11:20

Merge branch 'main' into issue-4906-prefer-nullish-enhancment

81ada98

Merge branch 'main' into issue-4906-prefer-nullish-enhancment

53bbfba

Merge branch 'main' into issue-4906-prefer-nullish-enhancment

d1b0347

Merge branch 'main' into issue-4906-prefer-nullish-enhancment

ef29982

JoshuaKGoldberg requested changes Jul 6, 2023

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jul 6, 2023

Omri Luzon added 3 commits July 8, 2023 09:27

flattening map

3747796

flattening map 2

4b6c03f

add examples to docs

7207e05

omril1 requested a review from JoshuaKGoldberg July 8, 2023 06:42

Merge remote-tracking branch 'upstream/main' into issue-4906-prefer-n…

5a18ee3

…ullish-enhancment

JoshuaKGoldberg removed the awaiting response Issues waiting for a reply from the OP or another party label Jul 8, 2023

JoshuaKGoldberg approved these changes Jul 8, 2023

View reviewed changes

JoshuaKGoldberg merged commit 6edaa04 into typescript-eslint:main Jul 8, 2023
35 of 41 checks passed

omril1 deleted the issue-4906-prefer-nullish-enhancment branch July 9, 2023 04:37

JoshuaKGoldberg mentioned this pull request Jul 10, 2023

Enhancement: [prefer-nullish-coalescing] Allow ignorePrimitives option to be true #7180

Closed

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [prefer-nullish-coalescing] add `ignorePrimitives` option #6487

feat(eslint-plugin): [prefer-nullish-coalescing] add `ignorePrimitives` option #6487

omril1 commented Feb 18, 2023 •

edited

typescript-eslint bot commented Feb 18, 2023

nx-cloud bot commented Feb 18, 2023 •

edited

netlify bot commented Feb 18, 2023 •

edited

codecov bot commented Feb 18, 2023 •

edited

JoshuaKGoldberg left a comment

JoshuaKGoldberg commented Mar 20, 2023

omril1 commented Mar 20, 2023

xsjcTony commented May 24, 2023

omril1 commented May 24, 2023

nx-cloud bot commented May 27, 2023 •

edited

omril1 commented Jun 16, 2023

JoshuaKGoldberg commented Jun 24, 2023 •

edited

JoshuaKGoldberg commented Jun 28, 2023

JoshuaKGoldberg left a comment

JoshuaKGoldberg Jul 6, 2023

omril1 Jul 8, 2023

JoshuaKGoldberg Jul 8, 2023

JoshuaKGoldberg Jul 6, 2023

omril1 Jul 6, 2023

omril1 Jul 8, 2023

JoshuaKGoldberg Jul 8, 2023 •

edited

JoshuaKGoldberg Jul 10, 2023

JoshuaKGoldberg Jul 6, 2023

omril1 Jul 6, 2023

JoshuaKGoldberg Jul 6, 2023

JoshuaKGoldberg left a comment

		@@ -131,6 +131,15 @@ a ?? (b && c && d);

		_NOTE:_ Errors for this specific case will be presented as suggestions (see below), instead of fixes. This is because it is not always safe to automatically convert `\|\|` to `??` within a mixed logical expression, as we cannot tell the intended precedence of the operator. Note that by design, `??` requires parentheses when used with `&&` or `\|\|` in the same expression.

		### `ignorePrimitives`

feat(eslint-plugin): [prefer-nullish-coalescing] add ignorePrimitives option #6487

feat(eslint-plugin): [prefer-nullish-coalescing] add ignorePrimitives option #6487

Conversation

omril1 commented Feb 18, 2023 • edited

PR Checklist

Overview

typescript-eslint bot commented Feb 18, 2023

nx-cloud bot commented Feb 18, 2023 • edited

☁️ Nx Cloud Report

netlify bot commented Feb 18, 2023 • edited

❌ Deploy Preview for typescript-eslint failed.

codecov bot commented Feb 18, 2023 • edited

Codecov Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Mar 20, 2023

omril1 commented Mar 20, 2023

xsjcTony commented May 24, 2023

omril1 commented May 24, 2023

nx-cloud bot commented May 27, 2023 • edited

☁️ Nx Cloud Report

omril1 commented Jun 16, 2023

JoshuaKGoldberg commented Jun 24, 2023 • edited

JoshuaKGoldberg commented Jun 28, 2023

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg Jul 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

feat(eslint-plugin): [prefer-nullish-coalescing] add `ignorePrimitives` option #6487

feat(eslint-plugin): [prefer-nullish-coalescing] add `ignorePrimitives` option #6487

omril1 commented Feb 18, 2023 •

edited

nx-cloud bot commented Feb 18, 2023 •

edited

netlify bot commented Feb 18, 2023 •

edited

codecov bot commented Feb 18, 2023 •

edited

nx-cloud bot commented May 27, 2023 •

edited

JoshuaKGoldberg commented Jun 24, 2023 •

edited

JoshuaKGoldberg Jul 8, 2023 •

edited