feat(eslint-plugin): apply final v6 changes to configs #7110

JoshuaKGoldberg · 2023-06-16T21:11:09Z

PR Checklist

Addresses an existing open issue: fixes Configs: Apply changes to config presets for v6 #6759
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Applies the final set of changes to preset configs for v6, as discussed in #6014.

typescript-eslint · 2023-06-16T21:11:12Z

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.

netlify · 2023-06-16T21:11:13Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`e8855fa`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/648f00b6847d080008cd23e0
😎 Deploy Preview	https://deploy-preview-7110--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 · 2023-06-16T21:26:48Z

Codecov Report

Merging #7110 (e8855fa) into v6 (89b14f2) will decrease coverage by 0.25%.
The diff coverage is 62.50%.

Additional details and impacted files

@@            Coverage Diff             @@
##               v6    #7110      +/-   ##
==========================================
- Coverage   87.75%   87.50%   -0.25%     
==========================================
  Files         372      372              
  Lines       12814    12849      +35     
  Branches     3800     3813      +13     
==========================================
- Hits        11245    11244       -1     
- Misses       1193     1227      +34     
- Partials      376      378       +2

Flag	Coverage Δ
unittest	`87.50% <62.50%> (-0.25%)`	⬇️

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

Impacted Files	Coverage Δ
...eslint-plugin/src/rules/consistent-type-exports.ts	`97.67% <ø> (ø)`
...kages/eslint-plugin/src/rules/no-base-to-string.ts	`98.38% <ø> (ø)`
...t-plugin/src/rules/no-confusing-void-expression.ts	`100.00% <ø> (ø)`
...plugin/src/rules/no-duplicate-type-constituents.ts	`100.00% <ø> (ø)`
...ackages/eslint-plugin/src/rules/no-implied-eval.ts	`96.92% <ø> (ø)`
...plugin/src/rules/no-redundant-type-constituents.ts	`92.36% <ø> (ø)`
packages/eslint-plugin/src/rules/no-shadow.ts	`77.65% <ø> (ø)`
...-plugin/src/rules/no-unsafe-declaration-merging.ts	`77.77% <ø> (ø)`
...lint-plugin/src/rules/no-unsafe-enum-comparison.ts	`100.00% <ø> (ø)`
...lint-plugin/src/rules/prefer-nullish-coalescing.ts	`96.61% <ø> (-3.39%)`	⬇️
... and 12 more

... and 28 files with indirect coverage changes

nx-cloud · 2023-06-16T21:32:43Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit e8855fa. 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 31 targets

Sent with 💌 from NxCloud.

packages/eslint-plugin/src/rules/consistent-type-exports.ts

packages/eslint-plugin/src/rules/no-base-to-string.ts

.eslintrc.js

packages/eslint-plugin/src/rules/no-base-to-string.ts

packages/eslint-plugin/src/rules/no-empty-interface.ts

packages/eslint-plugin/src/rules/consistent-type-exports.ts

packages/type-utils/src/TypeOrValueSpecifier.ts

packages/type-utils/src/isTypeReadonly.ts

packages/scope-manager/src/ScopeManager.ts

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

packages/eslint-plugin/src/rules/no-shadow.ts

packages/eslint-plugin/src/rules/no-redeclare.ts

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

bradzacher · 2023-07-22T23:48:47Z

packages/eslint-plugin/src/rules/restrict-plus-operands.ts

+      allowAny: true,
+      allowBoolean: true,
+      allowNullish: true,
+      allowNumberAndString: true,
+      allowRegExp: true,


@JoshuaKGoldberg i don't remember our discussion here - why did we turn all of the options on?
I thought we discussed that turning all the rule options on was bad as it made the rule effectively useless.

The issue was that it was chafing a lot of users by being very strict by default. A lot of folks are intentionally e.g. logging template literal strings with various primitives inside.

This does bring up the idea of having different configs in the strict ruleset than recommended... that would be a nice way to still get folks using the rule to its fullest if they want to be more strict. 🤔 #7318

Your example is template strings - but this is restrict-plus-operands.

So was this an accidental mix-up?

I think it applies to both of them. I've seen both happen with users. Which is why I keep mixing them up in comments 😅 to me, they're similar -just not exactly the same- cases.

I guess the question is - why even have these turned on at all if all the options are on by default?
The rules enforce pretty nothing like this so it's just wasted lint time.

Eg for RPO there's the following cases. Of these 16 cases the default options mean the rule only matches FOUR cases - two of which are TS already errors.

So we're recommending a rule to everyone to stop them from doing: string + object and string + unknown. The former is also banned under no-base-to-string, and one could argue the latter should be allowed under the allowAny option.

tl;dr - why are we even turning on the rules in the recommended set if they don't do anything with an overly permissive default option?

A couple reasons swayed me:

(lesser) There is benefit to string + unknown

(greater) I've seen a projects turn off no-base-to-string without turning off restrict-plus-operands

From a performance perspective, if one of these two rules is already asking for the types of these, I some level of caching on the TypeScript side would make the other rule not a significant change.

The thing I'd point to is that we added this big caution block to the RPO docs because we didn't think people using the options was good - and yet it's the default that everyone uses these options?
https://typescript-eslint.io/rules/restrict-plus-operands#:~:text=%2C%0A%5D%3B-,CAUTION,-We%20generally%20recommend

Just seems like we're contradicting ourselves "don't use these options but also they're all turned on by default"

Yeah :/ agreed. I don't like this.

Personal preference:
Given this rule is really doing nothing and a lot of people don't like it - we should just undo this change and only include it in strict-type-checked

feat(eslint-plugin): apply final v6 changes to configs

eb03fb4

Merge branch 'v6' into final-config-changes

632dee0

JoshuaKGoldberg requested a review from bradzacher June 16, 2023 21:26

Ah, fixed new lint rule violations internally

6e27cfc

JoshuaKGoldberg commented Jun 16, 2023

View reviewed changes

packages/eslint-plugin/src/rules/consistent-type-exports.ts Show resolved Hide resolved

JoshuaKGoldberg commented Jun 16, 2023

View reviewed changes

packages/eslint-plugin/src/rules/no-base-to-string.ts Show resolved Hide resolved

JoshuaKGoldberg added this to the 6.0.0 milestone Jun 17, 2023

JoshuaKGoldberg requested a review from a team June 17, 2023 21:44

bradzacher approved these changes Jun 18, 2023

View reviewed changes

JoshuaKGoldberg and others added 4 commits June 18, 2023 08:53

no-mixed-enums strict

d47cc0e

Apply suggestions from code review

97f8815

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

Fix up build & lint issues

380461d

Never mind, prefer nullish coalescing

e8855fa

JoshuaKGoldberg merged commit c13ce0b into typescript-eslint:v6 Jun 18, 2023

JoshuaKGoldberg deleted the final-config-changes branch June 18, 2023 13:25

This was referenced Jun 18, 2023

Repo: v6 canary releases workflow is failing #7111

Closed

Try out v6 beta on various important community repos #6760

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jun 26, 2023

bradzacher reviewed Jul 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): apply final v6 changes to configs #7110

feat(eslint-plugin): apply final v6 changes to configs #7110

JoshuaKGoldberg commented Jun 16, 2023

typescript-eslint bot commented Jun 16, 2023

netlify bot commented Jun 16, 2023 •

edited

Loading

codecov bot commented Jun 16, 2023 •

edited

Loading

nx-cloud bot commented Jun 16, 2023 •

edited

Loading

bradzacher Jul 22, 2023

JoshuaKGoldberg Jul 26, 2023

bradzacher Jul 26, 2023

JoshuaKGoldberg Jul 26, 2023

bradzacher Jul 26, 2023

JoshuaKGoldberg Jul 27, 2023

bradzacher Jul 28, 2023

JoshuaKGoldberg Jul 28, 2023

bradzacher Jul 28, 2023

feat(eslint-plugin): apply final v6 changes to configs #7110

feat(eslint-plugin): apply final v6 changes to configs #7110

Conversation

JoshuaKGoldberg commented Jun 16, 2023

PR Checklist

Overview

typescript-eslint bot commented Jun 16, 2023

netlify bot commented Jun 16, 2023 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

codecov bot commented Jun 16, 2023 • edited Loading

Codecov Report

nx-cloud bot commented Jun 16, 2023 • edited Loading

☁️ Nx Cloud Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Jun 16, 2023 •

edited

Loading

codecov bot commented Jun 16, 2023 •

edited

Loading

nx-cloud bot commented Jun 16, 2023 •

edited

Loading