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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check eslint-plugin-unicorn rules to enable/configure/disable #2439

Open
ST-DDT opened this issue Oct 7, 2023 · 47 comments · May be fixed by #3320
Open

Check eslint-plugin-unicorn rules to enable/configure/disable #2439

ST-DDT opened this issue Oct 7, 2023 · 47 comments · May be fixed by #3320
Assignees
Labels
c: infra Changes to our infrastructure or project setup p: 1-normal Nothing urgent
Milestone

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Oct 7, 2023

Followup on #2418

Currently all the following rules are disabled because they cause issues in the code:

Undecided

Rule Count Status/PR
unicorn/prevent-abbreviations 232 🤔 - #3320

Last updated: 2023-10-07

Rejected

Rule Count Status/PR
unicorn/import-style 8 🔴 - #2901 - doesn't do anything for us
unicorn/no-nested-ternary N/A 🔴 - incompatible with prettier
unicorn/no-null N/A 🔴 - incompatible with TypeScript
unicorn/no-object-as-default-parameter 6 🔴 - Unicorn-Issue
unicorn/no-zero-fractions 17 🔴 - #2453
unicorn/numeric-separators-style 131 🔴 - #2815
unicorn/number-literal-case N/A 🔴 - incompatible with prettier
unicorn/prefer-string-slice 75 🔴 - string.subtring is sometimes easier to understand - #3247 #2814
unicorn/prefer-ternary 8 🔴 - #2464

Accepted

Rule Count Status/PR
unicorn/catch-error-name 3 🟢 - #2471
unicorn/consistent-destructuring 4 🟢 - #2462
unicorn/consistent-function-scoping 12 🟢 - #3255 - Related: #2667
unicorn/escape-case 3 🟢 - #2469
unicorn/explicit-length-check 4 🟢 - #2455
unicorn/filename-case 1203 🟢 - #2492
unicorn/no-array-callback-reference 6 🟢 - #2722
unicorn/no-array-for-each 28 🟢 - #2461
unicorn/no-array-reduce 8 🟢 - #2479
unicorn/no-array-push-push 4 🟢 - #2454
unicorn/no-await-expression-member 6 🟢 - #2812
unicorn/no-console-spaces 2 🟢 - #2447
unicorn/no-for-loop 2 🟢 - #2490
unicorn/no-hex-escape 1 🟢 - #2440
unicorn/no-instanceof-array 1 🟢 - #2459
unicorn/no-negated-condition 2 🟢 - #2507
unicorn/no-new-array 1 🟢 - #2441
unicorn/no-process-exit 1 🟢 - #2448
unicorn/no-useless-switch-case 4 🟢 - #2508
unicorn/prefer-array-flat-map 2 🟢 - #2446
unicorn/prefer-array-some 1 🟢 - #2451
unicorn/prefer-at N/A 🟢 - #2654
unicorn/prefer-code-point 14 🟢 - #2509
unicorn/prefer-date-now N/A 🟢 - #2419
unicorn/prefer-export-from 74 🟢 - #3272
unicorn/prefer-includes 3 🟢 - #2463
unicorn/prefer-module 10 🟢 - #2510
unicorn/prefer-native-coercion-functions 1 🟢 - #2445
unicorn/prefer-negative-index 1 🟢 - #2512
unicorn/prefer-node-protocol N/A 🟢 - #2420
unicorn/prefer-number-properties 23 🟢 - #2452
unicorn/prefer-object-from-entries 3 🟢 - #2443
unicorn/prefer-optional-catch-binding 1 🟢 - #2491
unicorn/prefer-string-replace-all N/A 🟢 - #2653
unicorn/prefer-spread 51 🟢 - #2421
unicorn/prefer-string-starts-ends-with 1 🟢 - #2442
unicorn/prefer-top-level-await 4 🟢 - #2680
unicorn/require-array-join-separator N/A 🟢 - #2813
unicorn/switch-case-braces 51 🟢 - #2721
unicorn/text-encoding-identifier-case 3 🟢 - #2465

Please leave a comment if you wish to tackle any of these.


Legend:

  • 🤔 : This needs a decision whether we want to enable this.
  • 🔴 : We will not enable this rule.
  • 🟡 : We probably want to enable this rule, but maybe with additional configuration.
  • 🟢 : We want to enable this rule. Contributions welcome.
  • #1234 : Reference to the related PR.

The above list can be updated using the following command:

pnpm run lint | grep 'unicorn/' | sed 's/.*unicorn/unicorn/' | sort | uniq -c | sed -E 's/ *(.*) unicorn\/(.*)/\| [`unicorn\/\2`](https:\/\/github.com\/sindresorhus\/eslint-plugin-unicorn\/blob\/main\/docs\/rules\/\2.md) \| \1 \| 🤔 \|/'
@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision c: infra Changes to our infrastructure or project setup labels Oct 7, 2023
@ST-DDT ST-DDT added this to the vAnytime milestone Oct 7, 2023
@ST-DDT ST-DDT self-assigned this Oct 7, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 7, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 7, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 7, 2023

Created PR #2421 for unicorn/prefer-spread

Note: The rule is contested.

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 7, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 7, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 7, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 7, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 7, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 7, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 7, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 7, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 7, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 9, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 9, 2023

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 20, 2024

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 10, 2024

@faker-js/members, @faker-js/maintainers I'm unsure about the consistent function scoping one.

It basically wants us to move most inline functions to be standalone methods.

e.g. convert from

  dish(): string {
    const toTitleCase = (s: string) =>
      s
        .split(' ')
        .map((w) => w.charAt(0).toUpperCase() + w.slice(1))
        .join(' ');

    return toTitleCase(
      this.faker.helpers.arrayElement(this.faker.definitions.food.dish)
    );
  }

to

const toTitleCase = (s: string) =>
      s
        .split(' ')
        .map((w) => w.charAt(0).toUpperCase() + w.slice(1))
        .join(' ');

[...]

  dish(): string {
    return toTitleCase(
      this.faker.helpers.arrayElement(this.faker.definitions.food.dish)
    );
  }

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 10, 2024

Also, what are your thoughts regarding

?

@Shinigami92
Copy link
Member

@faker-js/members, @faker-js/maintainers I'm unsure about the consistent function scoping one.

It basically wants us to move most inline functions to be standalone methods.

e.g. convert from

  dish(): string {
    const toTitleCase = (s: string) =>
      s
        .split(' ')
        .map((w) => w.charAt(0).toUpperCase() + w.slice(1))
        .join(' ');

    return toTitleCase(
      this.faker.helpers.arrayElement(this.faker.definitions.food.dish)
    );
  }

to

const toTitleCase = (s: string) =>
      s
        .split(' ')
        .map((w) => w.charAt(0).toUpperCase() + w.slice(1))
        .join(' ');

[...]

  dish(): string {
    return toTitleCase(
      this.faker.helpers.arrayElement(this.faker.definitions.food.dish)
    );
  }

This makes sense, because instead of every single function call, only once the function will be created and than reused for multiple calls of the outer function.

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 10, 2024

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 19, 2024

@ST-DDT
Copy link
Member Author

ST-DDT commented Dec 3, 2024

unicorn/prevent-abbreviations is going to be a large PR and I'm not entirely convinced the variable renames help that much.

IMO i is a well accepted index keyword. And even if we would rename these cases we would still need lots of ignores.
I created a few small (maybe too small?) PRs to address some reasonable suggestions, we can discuss how to proceed, once these are merged.

@xDivisionByZerox
Copy link
Member

unicorn/prevent-abbreviations is going to be a large PR and I'm not entirely convinced the variable renames help that much.

IMO i is a well accepted index keyword. And even if we would rename these cases we would still need lots of ignores. I created a few small (maybe too small?) PRs to address some reasonable suggestions, we can discuss how to proceed, once these are merged.

I'm with you on this one. See my comment in #3315. While you could argue about some of the suggestions could be valid, I'd much rather see time spend on actual features or bug fixes for our users than having to fight in discussions about variable name nitpicking.

@ST-DDT ST-DDT linked a pull request Dec 3, 2024 that will close this issue
@ST-DDT ST-DDT linked a pull request Dec 3, 2024 that will close this issue
@ST-DDT
Copy link
Member Author

ST-DDT commented Dec 3, 2024

Finally 🥳

I created the last PR for this issue: #3320 for unicorn/prevent-abbreviations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: infra Changes to our infrastructure or project setup p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants