chore: enabled stylistic-type-checked internally #7138

JoshuaKGoldberg · 2023-06-25T01:53:33Z

PR Checklist

Addresses an existing open issue: continues Repo: Enable strict & stylistic configs internally #6827
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Adds stylistic-type-checked to the list of extends configs and removes manual enables of rules contained in it. Most of the changes seem to be from no-confusing-void-expression, followed by consistent-indexed-object-style.

A couple rules received special treatment:

Doesn't enable prefer-nullish-coalescing yet, as there were a lot of complaints from it and I'll look into it separately
no-empty-function seemed to be popular for tests, so I moved it to the tests overrides

typescript-eslint · 2023-06-25T01:53:36Z

Thanks for the PR, @JoshuaKGoldberg!

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-25T01:53:38Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`bc9238e`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/649f6417cba55300077aa9bf
😎 Deploy Preview	https://deploy-preview-7138--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 configuration.

nx-cloud · 2023-06-25T01:57:13Z

☁️ Nx Cloud Report

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

bradzacher · 2023-06-25T12:56:32Z

packages/eslint-plugin-internal/src/rules/plugin-test-formatting.ts

@@ -397,7 +402,7 @@ export default createRule<Options, MessageIds>({
              .join('\n')
          : formatted;

-        return context.report({


which rule suggested this?
TBH I prefer the return report style here as it's easier to see that the report is terminal.

no-confusing-void-expression. I'm feeling more and more 👎 on including that one in stylistic-type-checked.

bradzacher · 2023-06-25T13:04:50Z

packages/eslint-plugin/src/rules/naming-convention-utils/format.ts

@@ -17,26 +17,26 @@ https://gist.github.com/mathiasbynens/6334847
 function isPascalCase(name: string): boolean {
  return (
    name.length === 0 ||
-    (name[0] === name[0].toUpperCase() && !name.includes('_'))


I actually think that the previous form is clearer for this specifica case.
It's also substantially faster (not that it matters at the this speed... but it is twice as fast - ~1.2B ops/s vs ~2.4B ops/s).

I wonder if we should add an option to skip this case from the rule?

Agreed. The main 👎 on this change in my head was that the rule is suggesting longer code... #7140

bradzacher · 2023-06-25T13:06:28Z

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

+      ConditionalExpression: (node): void => {
+        checkNode(node.test);
+      },


TBH I personally don't think that multilining arrows is necessarily clearer.
Is there a reason the stylistic set is pushing for this style?

Isn’t this https://typescript-eslint.io/rules/no-confusing-void-expression ? You probably already know but posting for other readers :D

Yup it is! +1 to the visibility @rubiesonthesky 😄.

I wonder if it should be moved to the strict-type-checked config instead... #7130 (comment)

bradzacher · 2023-06-25T13:07:42Z

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

@@ -56,7 +56,7 @@ export function forEachReturnStatement<T>(
  function traverse(node: ts.Node): T | undefined {
    switch (node.kind) {
      case ts.SyntaxKind.ReturnStatement:
-        return visitor(<ts.ReturnStatement>node);


bradzacher · 2023-06-25T13:09:05Z

packages/rule-schema-to-typescript-types/src/index.ts

-  const defs = (schema.$defs ?? schema.definitions) as
-    | Record<string, JSONSchema4>
-    | undefined;


IIRC I did this intentionally because both of the keys were not techincally undefined - is that not the case any more?

Yup! no-unnecessary-type-assertion is complaining on them. Both schema.$defs and schema.definitions are Record<string, JSONSchema4> | undefined.

bradzacher · 2023-06-25T13:09:48Z

packages/rule-tester/src/types/DependencyConstraint.ts

-export interface DependencyConstraint {
-  /**
-   * Passing a string for the value is shorthand for a '>=' constraint
-   */
-  readonly [packageName: string]: VersionConstraint;
-}


Making this a record is actually less clear due to the lost comment

bradzacher · 2023-06-25T13:12:06Z

packages/scope-manager/src/scope/ScopeBase.ts

@@ -232,7 +232,7 @@ abstract class ScopeBase<
    block: TBlock,
    isMethodDefinition: boolean,
  ) {
-    const upperScopeAsScopeBase = upperScope as Scope;


I'm surprised TS allowed this - the reason this cast was there was because upperScope is typed as TUpper - and there was a lot of weird behaviour that stemmed from it being a generic instead of the concrete type.

I think there was also some weirdness due to the cyclic nature of the files? I don't fully remmeber though

bradzacher · 2023-06-25T13:15:44Z

packages/typescript-estree/tests/lib/convert.test.ts

@@ -21,73 +21,77 @@ describe('convert', () => {
    );
  }

-  it('deeplyCopy should convert node correctly', () => {
-    const ast = convertCode('type foo = ?foo<T> | ?(() => void)?');
+  /* eslint-disable @typescript-eslint/dot-notation */


why is this disabled?

deeplyCopy is a private member. ['deeplyCopy'] bypasses that.

bradzacher · 2023-06-25T13:17:05Z

packages/typescript-estree/tests/lib/persistentParse.test.ts

+    expect(() => {
+      parseFile('foo', PROJECT_DIR);
+    }).not.toThrow();
    // bar should throw because it doesn't exist yet
-    expect(() => parseFile(bazSlashBar, PROJECT_DIR)).toThrow();
+    expect(() => {
+      parseFile(bazSlashBar, PROJECT_DIR);
+    }).toThrow();


this is the sort of reason I think that the concise arrow function style is better - satisfying this lint rule just added some 160 lines to this file and (IMO) made the code harder to read

bradzacher · 2023-06-25T13:18:17Z

packages/utils/src/ts-eslint/Linter.ts

-  export interface GlobalsConfig {
-    [name: string]: GlobalVariableOption;
-  }
-  export interface EnvironmentConfig {
-    [name: string]: boolean;
-  }


I believe that these should stay as interfaces to allow for global augmentation from consumers.

bradzacher · 2023-06-25T13:20:29Z

packages/utils/src/ts-eslint/SourceCode.ts

-  export interface VisitorKeys {
-    [nodeType: string]: string[];
-  }


I do like the Record style more in general, but you can lose some nice implicit documentation sometimes (eg the variable name here).

I'm so torn about using a disable comment for this though because it's so minor.

It could use comment to say same if this style is kept :)

Yeah this is an interesting discussion. I'll undo the changes from consistent-indexed-object-style and file them as a todo to look at later, similar to prefer-nullish-coalescing.

packages/eslint-plugin/src/rules/prefer-string-starts-ends-with.ts

@@ -84,8 +84,7 @@ export default createRule({
      return (
        evaluated != null &&
        typeof evaluated.value === 'string' &&
-        // checks if the string is a character long
-        evaluated.value[0] === evaluated.value
+        evaluated.value.length === 1


JoshuaKGoldberg

(accidentally started a review, but getting pulled out for something - will continue commenting soon)

JoshuaKGoldberg · 2023-06-26T18:56:38Z

packages/eslint-plugin-internal/src/rules/plugin-test-formatting.ts

@@ -397,7 +402,7 @@ export default createRule<Options, MessageIds>({
              .join('\n')
          : formatted;

-        return context.report({


no-confusing-void-expression. I'm feeling more and more 👎 on including that one in stylistic-type-checked.

JoshuaKGoldberg · 2023-06-26T19:03:33Z

packages/eslint-plugin/src/rules/naming-convention-utils/format.ts

@@ -17,26 +17,26 @@ https://gist.github.com/mathiasbynens/6334847
 function isPascalCase(name: string): boolean {
  return (
    name.length === 0 ||
-    (name[0] === name[0].toUpperCase() && !name.includes('_'))


Agreed. The main 👎 on this change in my head was that the rule is suggesting longer code... #7140

JoshuaKGoldberg · 2023-06-26T19:05:12Z

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

+      ConditionalExpression: (node): void => {
+        checkNode(node.test);
+      },


Yup it is! +1 to the visibility @rubiesonthesky 😄.

I wonder if it should be moved to the strict-type-checked config instead... #7130 (comment)

codecov · 2023-06-26T22:48:22Z

Codecov Report

Merging #7138 (bc9238e) into v6 (3cdf5c9) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##               v6    #7138   +/-   ##
=======================================
  Coverage   87.43%   87.43%           
=======================================
  Files         372      372           
  Lines       12849    12849           
  Branches     3813     3813           
=======================================
  Hits        11235    11235           
  Misses       1237     1237           
  Partials      377      377

Flag	Coverage Δ
unittest	`87.43% <100.00%> (ø)`

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

Impacted Files	Coverage Δ
packages/eslint-plugin/src/rules/comma-dangle.ts	`93.33% <ø> (ø)`
...-estree/src/create-program/createDefaultProgram.ts	`76.19% <ø> (ø)`
.../src/create-program/getWatchProgramsForProjects.ts	`84.83% <ø> (ø)`
...pt-estree/src/parseSettings/createParseSettings.ts	`81.63% <ø> (ø)`
...plugin/src/rules/no-duplicate-type-constituents.ts	`100.00% <100.00%> (ø)`
...s/eslint-plugin/src/rules/no-restricted-imports.ts	`96.29% <100.00%> (ø)`
...lugin/src/rules/padding-line-between-statements.ts	`93.83% <100.00%> (ø)`
...nt-plugin/src/rules/switch-exhaustiveness-check.ts	`97.87% <100.00%> (ø)`
packages/eslint-plugin/src/util/astUtils.ts	`88.88% <100.00%> (ø)`
packages/scope-manager/src/scope/ScopeBase.ts	`91.71% <100.00%> (ø)`
... and 1 more

packages/typescript-estree/tests/lib/convert.test.ts

Co-authored-by: rubiesonthesky <2591240+rubiesonthesky@users.noreply.github.com>

chore: enabled stylistic-type-checked internally

6718c2f

JoshuaKGoldberg force-pushed the v6-stylistic-type-checked branch from 2a5e9f2 to 6718c2f Compare June 25, 2023 01:57

bradzacher reviewed Jun 25, 2023

View reviewed changes

rubiesonthesky reviewed Jun 26, 2023

View reviewed changes

JoshuaKGoldberg mentioned this pull request Jun 26, 2023

Enhancement: [prefer-string-starts-ends-with] Option to ignore [0] === cases? #7140

Closed

4 tasks

JoshuaKGoldberg commented Jun 26, 2023

View reviewed changes

JoshuaKGoldberg added 2 commits June 26, 2023 15:16

Reduced scope of changes

f237b10

Updated snapshots

335925c

JoshuaKGoldberg marked this pull request as ready for review June 26, 2023 23:18

JoshuaKGoldberg requested a review from bradzacher June 26, 2023 23:20

rubiesonthesky reviewed Jun 30, 2023

View reviewed changes

packages/typescript-estree/tests/lib/convert.test.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg and others added 3 commits June 30, 2023 16:22

Update packages/typescript-estree/tests/lib/convert.test.ts

9825cb0

Co-authored-by: rubiesonthesky <2591240+rubiesonthesky@users.noreply.github.com>

More deeplyCopy removals

a8fb72f

Updated test snapshots

bc9238e

JoshuaKGoldberg merged commit 42fe29f into typescript-eslint:v6 Jun 30, 2023

JoshuaKGoldberg deleted the v6-stylistic-type-checked branch June 30, 2023 23:53

github-actions bot locked as resolved and limited conversation to collaborators Jul 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: enabled stylistic-type-checked internally #7138

chore: enabled stylistic-type-checked internally #7138

JoshuaKGoldberg commented Jun 25, 2023

typescript-eslint bot commented Jun 25, 2023

netlify bot commented Jun 25, 2023 •

edited

Loading

nx-cloud bot commented Jun 25, 2023 •

edited

Loading

bradzacher Jun 25, 2023

JoshuaKGoldberg Jun 26, 2023

bradzacher Jun 25, 2023

JoshuaKGoldberg Jun 26, 2023

bradzacher Jun 25, 2023

rubiesonthesky Jun 26, 2023

JoshuaKGoldberg Jun 26, 2023

bradzacher Jun 25, 2023

bradzacher Jun 25, 2023

JoshuaKGoldberg Jun 26, 2023

bradzacher Jun 25, 2023

bradzacher Jun 25, 2023

JoshuaKGoldberg Jun 26, 2023

bradzacher Jun 25, 2023

JoshuaKGoldberg Jun 26, 2023

bradzacher Jun 25, 2023

bradzacher Jun 25, 2023

bradzacher Jun 25, 2023

rubiesonthesky Jun 26, 2023

JoshuaKGoldberg Jun 26, 2023

This comment was marked as off-topic.

JoshuaKGoldberg left a comment

JoshuaKGoldberg Jun 26, 2023

JoshuaKGoldberg Jun 26, 2023

JoshuaKGoldberg Jun 26, 2023

codecov bot commented Jun 26, 2023 •

edited

Loading

chore: enabled stylistic-type-checked internally #7138

chore: enabled stylistic-type-checked internally #7138

Conversation

JoshuaKGoldberg commented Jun 25, 2023

PR Checklist

Overview

typescript-eslint bot commented Jun 25, 2023

netlify bot commented Jun 25, 2023 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Jun 25, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

JoshuaKGoldberg left a 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

codecov bot commented Jun 26, 2023 • edited Loading

Codecov Report

netlify bot commented Jun 25, 2023 •

edited

Loading

nx-cloud bot commented Jun 25, 2023 •

edited

Loading

codecov bot commented Jun 26, 2023 •

edited

Loading