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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

jguddas
Copy link
Contributor

@jguddas jguddas commented May 12, 2022

PR Checklist

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

nx-cloud bot commented May 12, 2022

鈽侊笍 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
Copy link
Contributor

typescript-eslint bot commented May 12, 2022

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

netlify bot commented May 12, 2022

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...

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

codecov bot commented May 12, 2022

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

@jguddas 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
@tduyduc
Copy link
Contributor

tduyduc commented May 14, 2022

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

@jguddas jguddas force-pushed the feat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch from 1a36d4b to 1a02bdc Compare May 16, 2022
@jguddas
Copy link
Contributor Author

jguddas commented May 16, 2022

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?

@jguddas jguddas force-pushed the feat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch from da7c7c6 to f9e8c44 Compare May 16, 2022
@tduyduc
Copy link
Contributor

tduyduc commented May 16, 2022

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

@jguddas
Copy link
Contributor Author

jguddas commented May 16, 2022

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?

@bradzacher bradzacher added the enhancement: plugin rule option New rule option for an existing eslint-plugin rule label May 16, 2022
@bradzacher 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
@jguddas
Copy link
Contributor Author

jguddas commented May 19, 2022

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

@jguddas
Copy link
Contributor Author

jguddas commented May 24, 2022

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

bradzacher commented May 24, 2022

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

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

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.

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label May 25, 2022
@jguddas jguddas force-pushed the feat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch from d21395f to 39ea876 Compare May 25, 2022
@jguddas
Copy link
Contributor Author

jguddas commented May 25, 2022

@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.

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label May 25, 2022
@jguddas
Copy link
Contributor Author

jguddas commented May 25, 2022

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

@jguddas jguddas requested a review from JoshuaKGoldberg Jun 9, 2022
@jguddas jguddas force-pushed the feat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch from 2587035 to cafa047 Compare Jun 11, 2022
@Josh-Cena
Copy link
Contributor

Josh-Cena commented Jun 11, 2022

Just... don't force push

@jguddas
Copy link
Contributor Author

jguddas commented Jun 11, 2022

Just... don't force push

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

@jguddas jguddas closed this Jun 11, 2022
@jguddas jguddas reopened this Jun 11, 2022
@Josh-Cena
Copy link
Contributor

Josh-Cena commented Jun 11, 2022

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.

@jguddas jguddas requested a review from JoshuaKGoldberg Jun 11, 2022
@jguddas
Copy link
Contributor Author

jguddas commented Jun 11, 2022

This is in a presentable state now 馃帀

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Jun 11, 2022

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

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 13, 2022
) {
operator = '!=';
}
}
Copy link
Contributor Author

@jguddas jguddas Jun 13, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg Jul 23, 2022

Choose a reason for hiding this comment

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

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

@jguddas
Copy link
Contributor Author

jguddas commented Jun 22, 2022

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

JoshuaKGoldberg commented Jun 23, 2022

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?

packages/eslint-plugin/src/util/isNodeEqual.ts Outdated Show resolved Hide resolved
) {
operator = '!=';
}
}
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg Jul 23, 2022

Choose a reason for hiding this comment

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

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

JoshuaKGoldberg
JoshuaKGoldberg previously approved these changes Jul 23, 2022
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

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

@JoshuaKGoldberg JoshuaKGoldberg merged commit f82727f into typescript-eslint:main Jul 23, 2022
34 of 35 checks passed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jul 31, 2022
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 (@&#8203;typescript-eslint/eslint-plugin)</summary>

### [`v5.31.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#&#8203;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 ([#&#8203;5311](typescript-eslint/typescript-eslint#5311)) ([6d19efe](typescript-eslint/typescript-eslint@6d19efe))
-   **scope-manager:** handle typeParameters of TSInstantiationExpression ([#&#8203;5355](typescript-eslint/typescript-eslint#5355)) ([2595ccf](typescript-eslint/typescript-eslint@2595ccf))

##### Features

-   **eslint-plugin:** \[consistent-generic-ctors] check class field declaration ([#&#8203;5288](typescript-eslint/typescript-eslint#5288)) ([48f996e](typescript-eslint/typescript-eslint@48f996e))
-   **eslint-plugin:** \[prefer-nullish-coalescing] add ignoreTernaryTests option ([#&#8203;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 ([#&#8203;5342](typescript-eslint/typescript-eslint#5342)) ([98f6d5e](typescript-eslint/typescript-eslint@98f6d5e))
-   **eslint-plugin:** \[no-unused-vars] highlight last write reference ([#&#8203;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 [@&#8203;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 ([#&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 ([#&#8203;5270](typescript-eslint/typescript-eslint#5270)) ([d320174](typescript-eslint/typescript-eslint@58034e3))

</details>

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/parser)</summary>

### [`v5.31.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#&#8203;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 [@&#8203;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 ([#&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

#### 5.30.1 (2022-07-01)

**Note:** Version bump only for package [@&#8203;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.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMjcuMCIsInVwZGF0ZWRJblZlciI6IjMyLjEyNy4wIn0=-->

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>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[prefer-nullish-coalescing] request for ternary support
5 participants