Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

jguddas
Copy link
Contributor

PR Checklist

Overview

I added an option toprefer-nullish-coalescing (ignoreTernaryTests) which when set tofalse 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-cloudbot commentedMay 12, 2022•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit67e7ab5. 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 💌 fromNxCloud.

@typescript-eslint
Copy link
Contributor

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 onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day.

@netlify
Copy link

netlifybot commentedMay 12, 2022•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit67e7ab5
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/62dc1cf99a03170008e1c6e8
😎 Deploy Previewhttps://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 yourNetlify site settings.

@codecov
Copy link

codecovbot commentedMay 12, 2022•
edited
Loading

Codecov Report

Merging#4965 (3b36d89) intomain (78823cc) willincrease coverage by2.44%.
The diff coverage is97.75%.

❗ Current head3b36d89 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
FlagCoverage Δ
unittest93.80% <97.75%> (+2.44%)⬆️

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

Impacted FilesCoverage Δ
...lint-plugin/src/rules/prefer-nullish-coalescing.ts98.26% <96.92%> (ø)
...s/eslint-plugin/src/rules/prefer-optional-chain.ts93.85% <100.00%> (ø)
packages/eslint-plugin/src/util/isNodeEqual.ts100.00% <100.00%> (ø)
packages/eslint-plugin/src/util/isNullLiteral.ts100.00% <100.00%> (ø)
...es/eslint-plugin/src/util/isUndefinedIdentifier.ts100.00% <100.00%> (ø)
...s/eslint-plugin/src/rules/no-dupe-class-members.ts87.50% <0.00%> (ø)
...ackages/eslint-plugin/src/rules/keyword-spacing.ts100.00% <0.00%> (ø)
packages/eslint-plugin/src/rules/no-redeclare.ts91.89% <0.00%> (ø)
...es/eslint-plugin/src/rules/object-curly-spacing.ts100.00% <0.00%> (ø)
...t-plugin/src/rules/no-meaningless-void-operator.ts100.00% <0.00%> (ø)
... and150 more

@jguddasjguddasforce-pushed thefeat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch 5 times, most recently fromed7f382 to1a36d4bCompareMay 12, 2022 17:18
@tduyduc
Copy link
Contributor

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

bradzacher and jguddas reacted with thumbs up emoji

@jguddasjguddasforce-pushed thefeat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch from1a36d4b to1a02bdcCompareMay 16, 2022 07:52
@jguddas
Copy link
ContributorAuthor

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

@tduyduc great idea, can you take a look1a02bdc?

tduyduc reacted with thumbs up emoji

@jguddasjguddasforce-pushed thefeat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch fromda7c7c6 tof9e8c44CompareMay 16, 2022 08:24
@tduyduc
Copy link
Contributor

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

@jguddas
Copy link
ContributorAuthor

jguddas commentedMay 16, 2022•
edited
Loading

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?

@bradzacherbradzacher added the enhancement: plugin rule optionNew rule option for an existing eslint-plugin rule labelMay 16, 2022
@bradzacherbradzacher changed the titlefeat(eslint-plugin): added ignoreTernaryTests option to prefer-nullis…feat(eslint-plugin): [prefer-nullish-coalescing] add ignoreTernaryTests optionMay 16, 2022
@jguddas
Copy link
ContributorAuthor

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

@jguddas
Copy link
ContributorAuthor

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

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
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

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.

@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelMay 25, 2022
@jguddasjguddasforce-pushed thefeat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch fromd21395f to39ea876CompareMay 25, 2022 11:18
@jguddas
Copy link
ContributorAuthor

jguddas commentedMay 25, 2022•
edited
Loading

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

@bradzacherbradzacher removed the awaiting responseIssues waiting for a reply from the OP or another party labelMay 25, 2022
@jguddas
Copy link
ContributorAuthor

Funky edge case

The following code will log1.

leti=0constx={getx(){returni++}};console.log(x.x!=null ?x.x :y);// logs 1

But the fixed version with?? will log0.

leti=0constx={getx(){returni++}};console.log(x.x??y);// logs 0

@jguddasjguddasforce-pushed thefeat/eslint-plugin-added-ignoreTernaryTests-option-to-prefer-nullish-coalescing-rule branch from2587035 tocafa047CompareJune 11, 2022 14:04
@Josh-Cena
Copy link
Member

Just... don't force push

@jguddas
Copy link
ContributorAuthor

jguddas commentedJun 11, 2022•
edited
Loading

Just... don't force push

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

@jguddasjguddas reopened thisJun 11, 2022
@Josh-Cena
Copy link
Member

Josh-Cena commentedJun 11, 2022•
edited
Loading

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 reacted with thumbs up emoji

@jguddas
Copy link
ContributorAuthor

This is in a presentable state now 🎉

JoshuaKGoldberg reacted with hooray emoji

@JoshuaKGoldberg
Copy link
Member

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

@bradzacherbradzacher removed the awaiting responseIssues waiting for a reply from the OP or another party labelJun 13, 2022
) {
operator = '!=';
}
}
Copy link
ContributorAuthor

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 ofelse { return; } here instead of the oneif (!operator) { return; } at the end

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
ContributorAuthor

@JoshuaKGoldberg what do you think about changing the defaultignoreTernaryTests: true tofalse in a future major release?

But anyway, let's get this merged first 😄

@JoshuaKGoldberg
Copy link
Member

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 thebreaking changes label?

denolfe reacted with eyes emoji

) {
operator = '!=';
}
}

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 changesJul 23, 2022
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

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

@JoshuaKGoldbergJoshuaKGoldberg merged commitf82727f intotypescript-eslint:mainJul 23, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull requestJul 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/1480Reviewed-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-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsAug 23, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@tduyductduyductduyduc left review comments

@Josh-CenaJosh-CenaJosh-Cena left review comments

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
enhancement: plugin rule optionNew rule option for an existing eslint-plugin rule
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[prefer-nullish-coalescing] request for ternary support
5 participants
@jguddas@tduyduc@bradzacher@Josh-Cena@JoshuaKGoldberg
[8]ページ先頭
©2009-2025 Movatter.jp