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] addignorePrimitives option#6487

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

omril1
Copy link
Contributor

@omril1omril1 commentedFeb 18, 2023•
edited
Loading

PR Checklist

Overview

AddignorePrimitives option that can be set individually forstring,boolean,number andbigint

k-bialucha reacted with heart emoji
@typescript-eslint
Copy link
Contributor

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 onhttps://opencollective.com/typescript-eslint.

@nx-cloud
Copy link

nx-cloudbot commentedFeb 18, 2023•
edited
Loading

☁️ Nx Cloud Report

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

@netlify
Copy link

netlifybot commentedFeb 18, 2023•
edited
Loading

❌ Deploy Preview fortypescript-eslint failed.

NameLink
🔨 Latest commit5a18ee3
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/64a90610640a7600088db0e9

@omril1omril1 marked this pull request as draftFebruary 18, 2023 09:14
@omril1omril1 marked this pull request as ready for reviewFebruary 18, 2023 09:26
@codecov
Copy link

codecovbot commentedFeb 18, 2023•
edited
Loading

Codecov Report

Merging#6487 (5a18ee3) intomain (b1a23a9) willincrease coverage by0.00%.
The diff coverage is100.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
FlagCoverage Δ
unittest87.40% <100.00%> (+<0.01%)⬆️

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

Impacted FilesCoverage Δ
...lint-plugin/src/rules/prefer-nullish-coalescing.ts100.00% <100.00%> (ø)

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.

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

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

@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelFeb 18, 2023
@JoshuaKGoldberg
Copy link
Member

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

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

JoshuaKGoldberg reacted with thumbs up emoji

@bradzacherbradzacher added the enhancement: plugin rule optionNew rule option for an existing eslint-plugin rule labelApr 10, 2023
@xsjcTony
Copy link

Any updates here? 😄

@omril1
Copy link
ContributorAuthor

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.

xsjcTony, CyanSalt, and JoshuaKGoldberg reacted with heart emoji

@nx-cloud
Copy link

nx-cloudbot commentedMay 27, 2023•
edited
Loading

☁️ Nx Cloud Report

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

@omril1
Copy link
ContributorAuthor

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

JoshuaKGoldberg commentedJun 24, 2023•
edited
Loading

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!

omril1 reacted with hooray emoji

@JoshuaKGoldbergJoshuaKGoldberg removed the awaiting responseIssues waiting for a reply from the OP or another party labelJun 24, 2023
@JoshuaKGoldbergJoshuaKGoldberg dismissed theirstale reviewJune 24, 2023 18:21

Outdated review

@JoshuaKGoldberg
Copy link
Member

FWIW you don't have to keep mergingmain 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.

omril1 reacted with thumbs up emoji

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.

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

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

Choose a reason for hiding this comment

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

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

omril1 reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Good idea, leaving it for now.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldbergJul 8, 2023•
edited
Loading

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

#7180 ✨

omril1 reacted with rocket emoji
ignoreablePrimitive: ['boolean'],
literalPrimitive: 'true | false | null',
},
].map<TSESLint.InvalidTestCase<MessageIds, Options>>(

Choose a reason for hiding this comment

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

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

omril1 reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelJul 6, 2023
@JoshuaKGoldbergJoshuaKGoldberg removed the awaiting responseIssues waiting for a reply from the OP or another party labelJul 8, 2023
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.

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

omril1 reacted with hooray emojiomril1 reacted with heart emojiomril1 and CyanSalt reacted with rocket emoji
@JoshuaKGoldbergJoshuaKGoldberg merged commit6edaa04 intotypescript-eslint:mainJul 8, 2023
@omril1omril1 deleted the issue-4906-prefer-nullish-enhancment branchJuly 9, 2023 04:37
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsJul 18, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

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

Bug: [prefer-nullish-coalescing] False positive tonullable boolean [prefer-nullish-coalescing] request for preferring "||" when string value
4 participants
@omril1@JoshuaKGoldberg@xsjcTony@bradzacher
[8]ページ先頭
©2009-2025 Movatter.jp