feat(eslint-plugin): [prefer-nullish-coalescing] add ignoreTernaryTests option #4965

jguddas · 2022-05-12T15:07:52Z

PR Checklist

Addresses an existing open issue: fixes [prefer-nullish-coalescing] request for ternary support #4905
That issue was marked as accepting prs
Steps in CONTRIBUTING.md were taken

Overview

I added an option to prefer-nullish-coalescing (ignoreTernaryTests) which when set to false highlights ternary expressions that can be converted to use the nullish coalescing operator (??) instead.

If I have forgotten anything, or you have suggestions what I can do better please let me know, this is my first contribution here 😄

nx-cloud · 2022-05-12T15:07:53Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 67e7ab5. 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.

typescript-eslint · 2022-05-12T15:07:55Z

Thanks for the PR, @jguddas!

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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

netlify · 2022-05-12T15:08:03Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`67e7ab5`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/62dc1cf99a03170008e1c6e8
😎 Deploy Preview	https://deploy-preview-4965--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

codecov · 2022-05-12T15:17:11Z

Codecov Report

Merging #4965 (3b36d89) into main (78823cc) will increase coverage by 2.44%.
The diff coverage is 97.75%.

❗ Current head 3b36d89 differs from pull request most recent head 67e7ab5. Consider uploading reports for the commit 67e7ab5 to get more accurate results

@@            Coverage Diff             @@
##             main    #4965      +/-   ##
==========================================
+ Coverage   91.36%   93.80%   +2.44%     
==========================================
  Files         132      290     +158     
  Lines        1494     9967    +8473     
  Branches      226     2999    +2773     
==========================================
+ Hits         1365     9350    +7985     
- Misses         65      336     +271     
- Partials       64      281     +217

Flag	Coverage Δ
unittest	`93.80% <97.75%> (+2.44%)`	⬆️

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	`98.26% <96.92%> (ø)`
...s/eslint-plugin/src/rules/prefer-optional-chain.ts	`93.85% <100.00%> (ø)`
packages/eslint-plugin/src/util/isNodeEqual.ts	`100.00% <100.00%> (ø)`
packages/eslint-plugin/src/util/isNullLiteral.ts	`100.00% <100.00%> (ø)`
...es/eslint-plugin/src/util/isUndefinedIdentifier.ts	`100.00% <100.00%> (ø)`
...s/eslint-plugin/src/rules/no-dupe-class-members.ts	`87.50% <0.00%> (ø)`
...ackages/eslint-plugin/src/rules/keyword-spacing.ts	`100.00% <0.00%> (ø)`
packages/eslint-plugin/src/rules/no-redeclare.ts	`91.89% <0.00%> (ø)`
...es/eslint-plugin/src/rules/object-curly-spacing.ts	`100.00% <0.00%> (ø)`
...t-plugin/src/rules/no-meaningless-void-operator.ts	`100.00% <0.00%> (ø)`
... and 150 more

tduyduc · 2022-05-14T16:07:30Z

How about the == operator where null and undefined are equated (for example: null != x ? x : 'a default value')?

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

jguddas · 2022-05-16T07:55:09Z

How about the == operator where null and undefined are equated (for example: null != x ? x : 'a default value')?

@tduyduc great idea, can you take a look 1a02bdc?

tduyduc · 2022-05-16T14:26:02Z

Not sure about the rule source code, but the tests look great to me! 👍

jguddas · 2022-05-16T14:51:18Z

MemberExpressions should probably also be covered 🤔 .

x.x != null ? x.x : y;
x[1][2][3][4] != null ? x[1][2][3][4] : y;

declare const x: { x: null | string };
x.x !== null ? x.x : y;

Is there a utility that alrealy exists that allows me to compare member expressions?

jguddas · 2022-05-19T14:25:58Z

I just added support for MemberExpressions, it works great, but the code is sadly running a bit out of hand 🙁

jguddas · 2022-05-24T13:37:15Z

I would love some feedback on the functionality and know what besides cleaning up the code needs to be done to get this merged, are there any edge cases I have forgotten to cover?

bradzacher · 2022-05-24T23:52:54Z

Hi @jguddas! This PR is in the queue of PRs to reviewed, and will be re-reviewed as soon as we are able.
https://github.com/typescript-eslint/typescript-eslint/blob/master/CONTRIBUTING.md#addressing-feedback-and-beyond

JoshuaKGoldberg

You're right, this is getting to be a lot of code 😄. But that feels reasonable, this is a very tricky change you're tackling. Lots of edge cases! Nice job getting it this far 👏.

There are also a lot of missing cases as pointed out by the TypeScript errors & codecov complaints. In good TDD form, I'd suggest you fill in all those tests so we can get a feel for what the code needs to do. Then it should be more clear what can or can't be refactored.

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

jguddas · 2022-05-25T11:38:10Z

@JoshuaKGoldberg I cleaned this up a bit, maybe you can take a look again.

There are also a lot of missing cases as pointed out by the TypeScript errors & codecov complaints.

Yeah, those errors are super helpful but look worse than they are, I fixed this by adding 2 new test case and deleting one unreachable/duplicated check.

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

packages/eslint-plugin/tests/util/isNodeEqual.test.ts

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

jguddas · 2022-05-25T18:51:38Z

Funky edge case

The following code will log 1.

let i = 0
const x = { get x() { return i++ } };
console.log (x.x != null ? x.x : y); // logs 1

But the fixed version with ?? will log 0.

let i = 0
const x = { get x() { return i++ } };
console.log (x.x ?? y); // logs 0

Josh-Cena · 2022-06-11T14:05:51Z

Just... don't force push

jguddas · 2022-06-11T14:09:52Z

Just... don't force push

Okay, do you @Josh-Cena prefer merge commits from main to this branch instead of rebasing?

Josh-Cena · 2022-06-11T14:20:47Z

At least that's what's said in the CONTRIBUTING guides... I'm not a maintainer/reviewer anyway, and my reviewing habit doesn't include doing incremental reviews. But the TS-ESLint maintainers do use that, so it's best to respect their workflow.

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

jguddas · 2022-06-11T14:59:57Z

This is in a presentable state now 🎉

JoshuaKGoldberg · 2022-06-11T15:25:04Z

Re the force pushing: yup, it's not a blocker for review, but does make it a little harder on our end. Thanks 😄 #5170

jguddas · 2022-06-13T09:19:26Z

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

+            ) {
+              operator = '!=';
+            }
+          }


The other way to solve this is to put a bunch of else { return; } here instead of the one if (!operator) { return; } at the end

Yeah I tried refactoring a bit and couldn't find anything significantly cleaner. 🤷

jguddas · 2022-06-22T20:38:30Z

@JoshuaKGoldberg what do you think about changing the default ignoreTernaryTests: true to false in a future major release?

But anyway, let's get this merged first 😄

JoshuaKGoldberg · 2022-06-23T14:11:03Z

Sorry for taking so long to re-review! I've been a little swamped this month (book release, conferences, etc.). But this is still on the backlog!

default ignoreTernaryTests: true to false in a future major release

Yeah I like that 🙂 agreed. Would you be open to filing a new issue so we can track with the breaking changes label?

…option-to-prefer-nullish-coalescing-rule

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

packages/eslint-plugin/src/util/isNodeEqual.ts

JoshuaKGoldberg · 2022-07-23T16:07:25Z

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

+            ) {
+              operator = '!=';
+            }
+          }


Yeah I tried refactoring a bit and couldn't find anything significantly cleaner. 🤷

JoshuaKGoldberg

Yup, this looks great - thanks for all your hard work on this @jguddas!

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.30.7` -> `5.31.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.30.7/5.31.0) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.30.7` -> `5.31.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.30.7/5.31.0) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/eslint-plugin)</summary> ### [`v5.31.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5310-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5307v5310-2022-07-25) [Compare Source](typescript-eslint/typescript-eslint@v5.30.7...v5.31.0) ##### Bug Fixes - **eslint-plugin:** \[typedef] Support nested array destructuring with type annotation ([#5311](typescript-eslint/typescript-eslint#5311)) ([6d19efe](typescript-eslint/typescript-eslint@6d19efe)) - **scope-manager:** handle typeParameters of TSInstantiationExpression ([#5355](typescript-eslint/typescript-eslint#5355)) ([2595ccf](typescript-eslint/typescript-eslint@2595ccf)) ##### Features - **eslint-plugin:** \[consistent-generic-ctors] check class field declaration ([#5288](typescript-eslint/typescript-eslint#5288)) ([48f996e](typescript-eslint/typescript-eslint@48f996e)) - **eslint-plugin:** \[prefer-nullish-coalescing] add ignoreTernaryTests option ([#4965](typescript-eslint/typescript-eslint#4965)) ([f82727f](typescript-eslint/typescript-eslint@f82727f)) #### [5.30.7](typescript-eslint/typescript-eslint@v5.30.6...v5.30.7) (2022-07-18) ##### Bug Fixes - **eslint-plugin:** \[no-inferrable] fix optional param to valid code ([#5342](typescript-eslint/typescript-eslint#5342)) ([98f6d5e](typescript-eslint/typescript-eslint@98f6d5e)) - **eslint-plugin:** \[no-unused-vars] highlight last write reference ([#5267](typescript-eslint/typescript-eslint#5267)) ([c3f199a](typescript-eslint/typescript-eslint@c3f199a)) #### [5.30.6](typescript-eslint/typescript-eslint@v5.30.5...v5.30.6) (2022-07-11) **Note:** Version bump only for package [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/eslint-plugin) #### [5.30.5](typescript-eslint/typescript-eslint@v5.30.4...v5.30.5) (2022-07-04) ##### Bug Fixes - **eslint-plugin:** \[consistent-indexed-object-style] fix record mode fixer for generics with a default value ([#5280](typescript-eslint/typescript-eslint#5280)) ([57f032c](typescript-eslint/typescript-eslint@57f032c)) #### [5.30.4](typescript-eslint/typescript-eslint@v5.30.3...v5.30.4) (2022-07-03) **Note:** Version bump only for package [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/eslint-plugin) #### [5.30.3](typescript-eslint/typescript-eslint@v5.30.2...v5.30.3) (2022-07-01) **Note:** Version bump only for package [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/eslint-plugin) #### [5.30.2](typescript-eslint/typescript-eslint@v5.30.1...v5.30.2) (2022-07-01) **Note:** Version bump only for package [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/eslint-plugin) #### [5.30.1](typescript-eslint/typescript-eslint@v5.30.0...v5.30.1) (2022-07-01) ##### Bug Fixes - **eslint-plugin:** \[no-base-to-string] add missing apostrophe to message ([#5270](typescript-eslint/typescript-eslint#5270)) ([d320174](typescript-eslint/typescript-eslint@58034e3)) </details> <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/parser)</summary> ### [`v5.31.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5310-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5307v5310-2022-07-25) [Compare Source](typescript-eslint/typescript-eslint@v5.30.7...v5.31.0) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) #### [5.30.7](typescript-eslint/typescript-eslint@v5.30.6...v5.30.7) (2022-07-18) ##### Bug Fixes - expose types supporting old versions of typescript ([#5339](typescript-eslint/typescript-eslint#5339)) ([4ba9bdb](typescript-eslint/typescript-eslint@4ba9bdb)) #### [5.30.6](typescript-eslint/typescript-eslint@v5.30.5...v5.30.6) (2022-07-11) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) #### [5.30.5](typescript-eslint/typescript-eslint@v5.30.4...v5.30.5) (2022-07-04) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) #### [5.30.4](typescript-eslint/typescript-eslint@v5.30.3...v5.30.4) (2022-07-03) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) #### [5.30.3](typescript-eslint/typescript-eslint@v5.30.2...v5.30.3) (2022-07-01) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) #### [5.30.2](typescript-eslint/typescript-eslint@v5.30.1...v5.30.2) (2022-07-01) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) #### 5.30.1 (2022-07-01) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1480 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

jguddas force-pushed the feat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch 5 times, most recently from ed7f382 to 1a36d4b Compare May 12, 2022 17:18

tduyduc reviewed May 14, 2022

View reviewed changes

packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md Outdated Show resolved Hide resolved

jguddas force-pushed the feat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch from 1a36d4b to 1a02bdc Compare May 16, 2022 07:52

Josh-Cena reviewed May 16, 2022

View reviewed changes

packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md Outdated Show resolved Hide resolved

jguddas force-pushed the feat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch from da7c7c6 to f9e8c44 Compare May 16, 2022 08:24

bradzacher added the enhancement: plugin rule option New rule option for an existing eslint-plugin rule label May 16, 2022

bradzacher changed the title ~~feat(eslint-plugin): added ignoreTernaryTests option to prefer-nullis…~~ feat(eslint-plugin): [prefer-nullish-coalescing] add ignoreTernaryTests option May 16, 2022

JoshuaKGoldberg requested changes May 25, 2022

View reviewed changes

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts Outdated Show resolved Hide resolved

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label May 25, 2022

jguddas force-pushed the feat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch from d21395f to 39ea876 Compare May 25, 2022 11:18

jguddas commented May 25, 2022

View reviewed changes

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts Outdated Show resolved Hide resolved

packages/eslint-plugin/tests/util/isNodeEqual.test.ts Show resolved Hide resolved

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts Outdated Show resolved Hide resolved

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label May 25, 2022

jguddas requested a review from JoshuaKGoldberg June 9, 2022 11:25

jguddas force-pushed the feat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch from 2587035 to cafa047 Compare June 11, 2022 14:04

jguddas closed this Jun 11, 2022

jguddas reopened this Jun 11, 2022

jguddas commented Jun 11, 2022

View reviewed changes

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts Outdated Show resolved Hide resolved

jguddas added 2 commits June 11, 2022 16:28

test: added more test cases

8ee505c

fix: fixed tests

119f14d

jguddas requested a review from JoshuaKGoldberg June 11, 2022 14:33

jguddas added 2 commits June 11, 2022 16:39

chore: fixed typo

31966ff

test: added more test cases

3d0eab1

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 13, 2022

jguddas added 2 commits June 13, 2022 11:11

refactor: inlined getOperator and getNodes

73034b0

perf: skip type check if both null and undefined are not checked

4ef1ef7

jguddas commented Jun 13, 2022

View reviewed changes

Merge branch 'main' into feat/eslint-plugin-added-ignoreTernaryTests-…

3b36d89

…option-to-prefer-nullish-coalescing-rule

JoshuaKGoldberg reviewed Jul 23, 2022

View reviewed changes

JoshuaKGoldberg previously approved these changes Jul 23, 2022

View reviewed changes

Apply suggestions from code review

67e7ab5

JoshuaKGoldberg dismissed their stale review via 67e7ab5 July 23, 2022 16:08

JoshuaKGoldberg enabled auto-merge (squash) July 23, 2022 16:09

JoshuaKGoldberg approved these changes Jul 23, 2022

View reviewed changes

JoshuaKGoldberg merged commit f82727f into typescript-eslint:main Jul 23, 2022

github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [prefer-nullish-coalescing] add ignoreTernaryTests option #4965

feat(eslint-plugin): [prefer-nullish-coalescing] add ignoreTernaryTests option #4965

jguddas commented May 12, 2022

nx-cloud bot commented May 12, 2022 •

edited

Loading

typescript-eslint bot commented May 12, 2022

netlify bot commented May 12, 2022 •

edited

Loading

codecov bot commented May 12, 2022 •

edited

Loading

tduyduc commented May 14, 2022

jguddas commented May 16, 2022

tduyduc commented May 16, 2022

jguddas commented May 16, 2022 •

edited

Loading

jguddas commented May 19, 2022

jguddas commented May 24, 2022

bradzacher commented May 24, 2022

JoshuaKGoldberg left a comment

jguddas commented May 25, 2022 •

edited

Loading

jguddas commented May 25, 2022

Josh-Cena commented Jun 11, 2022

jguddas commented Jun 11, 2022 •

edited

Loading

Josh-Cena commented Jun 11, 2022 •

edited

Loading

jguddas commented Jun 11, 2022

JoshuaKGoldberg commented Jun 11, 2022

jguddas Jun 13, 2022

JoshuaKGoldberg Jul 23, 2022

jguddas commented Jun 22, 2022

JoshuaKGoldberg commented Jun 23, 2022

JoshuaKGoldberg Jul 23, 2022

JoshuaKGoldberg left a comment

feat(eslint-plugin): [prefer-nullish-coalescing] add ignoreTernaryTests option #4965

feat(eslint-plugin): [prefer-nullish-coalescing] add ignoreTernaryTests option #4965

Conversation

jguddas commented May 12, 2022

PR Checklist

Overview

nx-cloud bot commented May 12, 2022 • edited Loading

☁️ Nx Cloud Report

typescript-eslint bot commented May 12, 2022

netlify bot commented May 12, 2022 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

codecov bot commented May 12, 2022 • edited Loading

Codecov Report

tduyduc commented May 14, 2022

jguddas commented May 16, 2022

tduyduc commented May 16, 2022

jguddas commented May 16, 2022 • edited Loading

jguddas commented May 19, 2022

jguddas commented May 24, 2022

bradzacher commented May 24, 2022

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

jguddas commented May 25, 2022 • edited Loading

jguddas commented May 25, 2022

Funky edge case

Josh-Cena commented Jun 11, 2022

jguddas commented Jun 11, 2022 • edited Loading

Josh-Cena commented Jun 11, 2022 • edited Loading

jguddas commented Jun 11, 2022

JoshuaKGoldberg commented Jun 11, 2022

jguddas Jun 13, 2022

Choose a reason for hiding this comment

JoshuaKGoldberg Jul 23, 2022

Choose a reason for hiding this comment

jguddas commented Jun 22, 2022

JoshuaKGoldberg commented Jun 23, 2022

JoshuaKGoldberg Jul 23, 2022

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

nx-cloud bot commented May 12, 2022 •

edited

Loading

netlify bot commented May 12, 2022 •

edited

Loading

codecov bot commented May 12, 2022 •

edited

Loading

jguddas commented May 16, 2022 •

edited

Loading

jguddas commented May 25, 2022 •

edited

Loading

jguddas commented Jun 11, 2022 •

edited

Loading

Josh-Cena commented Jun 11, 2022 •

edited

Loading