Repo: enable more `eslint-plugin-unicorn` rules internally #9623

abrahamguo · 2024-07-23T12:52:27Z

Suggestion

Following up on #9523, I was looking through the eslint-plugin-unicorn rules that we do not have turned on internally, and I found these rules that might be beneficial to enable. Thoughts?

better-regex (26 occurrences)

docs

/([`$``])/g can be optimized to /([$```])/g.
eslint-plugin/src/rules/no-unnecessary-template-expression.ts:150
eslint-plugin/src/rules/no-useless-template-literals.ts:151

/(^|[^\`\`])"(?:``.|[^\`\`"\`r\`n])`"(?!`s`:)/ can be optimized to /(^|[^\`\`])"(?:``.|[^\`n\`r"\`\`])`"(?!`s`:)/.
website/src/prism/language/jsonc.js:11

/(^|[^\`\`])"(?:``.|[^\`\`"\`r\`n])`"(?=`s`:)/ can be optimized to /(^|[^\`\`])"(?:``.|[^\`n\`r"\`\`])`"(?=`s`:)/.
website/src/prism/language/jsonc.js:6

/[{}[`],]/ can be optimized to /[,[`]{}]/.
website/src/prism/language/jsonc.js:20

/[``/]`error`[``/]/ can be optimized to /[/``]`error`[/``]/.
ast-spec/tests/fixtures.test.ts:75

/[``^$.`+?()[`]{}|]/g can be optimized to /[$()`+.?[```]^{|}]/g.
eslint-plugin/src/util/escapeRegExp.ts:5

/`/`/.`|`/``[`s`S]`?(?:```/|$)/ can be optimized to /`/`/.`|`/``[`S`s]`?(?:```/|$)/.
website/src/prism/language/jsonc.js:16

/``(?!["])/gm can be optimized to /``(?!")/gm.
typescript-estree/tests/lib/parse.test.ts:60

/`$`{/g can be optimized to /`${/g.
eslint-plugin-internal/src/rules/plugin-test-formatting.ts:51

/`r`n|[`r`n`u2028`u2029]/ can be optimized to /`r`n|[`n`r`u2028`u2029]/.
utils/src/ast-utils/misc.ts:3

/&(?:#`d+|#x[`da-fA-F]+|[0-9a-zA-Z]+);/g can be optimized to /&(?:#`d+|#x[`dA-Fa-f]+|[`dA-Za-z]+);/g.
typescript-estree/src/node-utils.ts:450

/^([ ]`)/ can be optimized to /^( `)/.
eslint-plugin-internal/src/rules/plugin-test-formatting.ts:49

/^(`/?|)([`s`S]`?)((?:`.{1,2}|[^/]+?|)(`.[^./]`|))(?:[/]`)$/ can be optimized to /^(`/?|)([`S`s]`?)((?:`.{1,2}|[^/]+?|)(`.[^./]`|))`/`$/.
website-eslint/src/mock/path.js:55

/^"'['"]$/ can be optimized to /^"'["']$/.
scope-manager/tests/fixtures.test.ts:39

/^[(`w+:)``/~~]/ can be optimized to /^[`w()+/:``~~]/.
typescript-estree/src/create-program/describeFilePath.ts:18

/^[`([]/ can be optimized to /^[([`]/.
eslint-plugin/src/util/getWrappingFixer.ts:70

/^[=!]=/ can be optimized to /^[!=]=/.
eslint-plugin/src/rules/prefer-string-starts-ends-with.ts:16

/^[=?:]$/ can be optimized to /^[:=?]$/.
eslint-plugin/src/rules/space-infix-ops.ts:74

/^`/`/`/`/[ ]+@(`w+)[ ]`=[ ]`(.+)$/ can be optimized to /^`/{4} +@(`w+) `= `(.+)$/.
scope-manager/tests/fixtures.test.ts:38

/^`s`(?:`/|``)``s`@ts-(?expect-error|ignore)(?.`)/ can be optimized to /^`s`[`/]``s`@ts-(?expect-er{2}or|ignore)(?.`)/.
eslint-plugin/src/rules/ban-ts-comment.ts:117

/^`s`(?:`/|``)``s`@ts-ignore/ can be optimized to /^`s`[`/]``s`@ts-ignore/.
eslint-plugin/src/rules/prefer-ts-expect-error.ts:28

/Cannot read file .+semanticInfo'/i can be optimized to /cannot read file .+semanticinfo'/i.
typescript-estree/tests/lib/semanticInfo.test.ts:286

/getParserServices(`(`s`[^,\`s)]+)`s`(,`s`false`s`)?`)/ can be optimized to /getParserServices(`(`s`[^\`s),]+)`s`(,`s`false`s`)?`)/.
eslint-plugin/tests/docs.test.ts:512

/import`s``{`s`TypeScriptWorker`s`}`s`from`s`['"].`/tsWorker['"];/ can be optimized to /import`s`{`s`TypeScriptWorker`s`}`s`from`s`["'].`/tsWorker["'];/.
website/tools/generate-website-dts.ts:75

catch-error-name (29 occurrences)

docs

The catch parameter err should be named error.
rule-tester/src/RuleTester.ts:622
rule-tester/src/utils/config-validator.ts:116
rule-tester/src/utils/config-validator.ts:197
website/plugins/generated-rule-docs/addESLintHashToCodeBlocksMeta.ts:57
website/src/components/editor/useSandboxServices.ts:137

The catch parameter ex should be named error.
eslint-plugin-internal/src/rules/plugin-test-formatting.ts:171
integration-tests/tools/integration-test-base.ts:155

custom-error-definition (4 occurrences)

docs

The name property should be set to AssertionError.
website-eslint/src/mock/assert.js:27

The name property should be set to NotSupportedError.
rule-schema-to-typescript-types/src/errors.ts:2

The name property should be set to TSError.
typescript-estree/src/node-utils.ts:702

The name property should be set to UnexpectedError.
rule-schema-to-typescript-types/src/errors.ts:14

error-message (1 occurrence)

docs

Pass a message to the Error constructor.
website-eslint/src/mock/assert.js:45

no-array-for-each (118 occurrences)

docs

no-array-method-this-argument (4 occurrences)

docs

Do not use the this argument in Array#findIndex().
website/plugins/generated-rule-docs/insertions/insertWhenNotToUseIt.ts:18

Do not use the this argument in Array#forEach().
scope-manager/src/referencer/PatternVisitor.ts:59
scope-manager/src/referencer/TypeVisitor.ts:191
scope-manager/src/referencer/Visitor.ts:40

no-array-push-push (22 occurrences)

docs

no-array-reduce (29 occurrences)

docs

no-console-spaces (1 occurrence)

docs

Do not use trailing space between console.log parameters.
repo-tools/src/apply-canary-version.mts:35

no-for-loop (21 occurrences)

docs

no-hex-escape (10 occurrences)

docs

Use Unicode escapes instead of hexadecimal escapes.
repo-tools/src/generate-configs.mts:22
repo-tools/src/generate-configs.mts:22
repo-tools/src/generate-configs.mts:23
repo-tools/src/generate-configs.mts:23
repo-tools/src/generate-configs.mts:24
repo-tools/src/generate-configs.mts:24
repo-tools/src/generate-configs.mts:25
repo-tools/src/generate-configs.mts:25
repo-tools/src/generate-configs.mts:26
repo-tools/src/generate-configs.mts:26

no-lonely-if (24 occurrences)

docs

no-object-as-default-parameter (1 occurrence)

docs

Do not use an object literal as default for parameter options.
scope-manager/src/referencer/Visitor.ts:31

no-unused-properties (2 occurrences)

docs

Property emitDecoratorMetadata is defined but never used.
scope-manager/src/analyze.ts:75

Property lib is defined but never used.
scope-manager/src/analyze.ts:73

no-useless-spread (3 occurrences)

docs

Spread an object literal in object literal is unnecessary.
eslint-plugin/src/rules/naming-convention-utils/schema.ts:149
eslint-plugin/src/rules/type-annotation-spacing.ts:49
eslint-plugin/src/rules/type-annotation-spacing.ts:54

no-useless-switch-case (6 occurrences)

docs

Useless case in switch statement.
typescript-estree/src/convert.ts:651
typescript-estree/src/convert.ts:652
typescript-estree/src/node-utils.ts:591
typescript-estree/src/node-utils.ts:592
typescript-estree/src/node-utils.ts:593
typescript-estree/src/node-utils.ts:594

no-useless-undefined (17 occurrences)

docs

Do not use useless undefined.
eslint-plugin/src/rules/no-restricted-imports.ts:101
eslint-plugin/src/rules/no-restricted-imports.ts:109
eslint-plugin/src/rules/no-restricted-imports.ts:136
parser/tests/test-utils/test-utils.ts:25
typescript-estree/src/create-program/createIsolatedProgram.ts:54
typescript-estree/src/simple-traverse.ts:91
typescript-estree/tests/lib/createProjectService.test.ts:26
typescript-estree/tests/lib/createProjectService.test.ts:35
typescript-estree/tests/lib/createProjectService.test.ts:60
typescript-estree/tests/lib/createProjectService.test.ts:78
typescript-estree/tests/lib/createProjectService.test.ts:92
typescript-estree/tests/lib/createProjectService.test.ts:105
typescript-estree/tests/lib/useProgramFromProjectService.test.ts:211
website/src/components/ESQueryFilter.tsx:24
website/src/components/ast/DataRenderer.tsx:68
website/src/components/ast/DataRenderer.tsx:200
website/src/components/typeDetails/TypeInfo.tsx:71

number-literal-case (5 occurrences)

docs

Invalid number literal casing.
eslint-plugin/tests/rules/no-magic-numbers.test.ts:146
eslint-plugin/tests/rules/no-magic-numbers.test.ts:150
eslint-plugin/tests/rules/no-magic-numbers.test.ts:807
eslint-plugin/tests/rules/no-magic-numbers.test.ts:821
typescript-estree/src/node-utils.ts:457

prefer-add-event-listener (1 occurrence)

docs

Prefer addEventListener over onload.
website/src/components/editor/loadSandbox.ts:20

The text was updated successfully, but these errors were encountered:

abrahamguo · 2024-07-23T12:52:40Z

prefer-array-flat (1 occurrence)

docs

Prefer Array#flat() over Array#reduce() to flatten an array.
eslint-plugin/tests/rules/no-inferrable-types.test.ts:14

prefer-array-some (1 occurrence)

docs

Prefer .some(…) over .find(…).
eslint-plugin/src/rules/no-unnecessary-condition.ts:580

prefer-at (50 occurrences)

docs

Prefer .at(…) over the first element from .slice(…).
eslint-plugin/src/util/misc.ts:181

prefer-code-point (2 occurrences)

docs

Prefer String#codePointAt() over String#charCodeAt().
type-utils/src/requiresQuoting.ts:11
type-utils/src/requiresQuoting.ts:16

prefer-dom-node-append (1 occurrence)

docs

Prefer Node#append() over Node#appendChild().
website/src/components/editor/loadSandbox.ts:47

prefer-export-from (6 occurrences)

docs

Use export…from to re-export ASTUtils.
utils/src/index.ts:7

Use export…from to re-export ConfigWithExtends.
typescript-eslint/src/index.ts:72

Use export…from to re-export ESLintUtils.
utils/src/index.ts:7

Use export…from to re-export JSONSchema.
utils/src/index.ts:7

Use export…from to re-export TSESLint.
utils/src/index.ts:7

Use export…from to re-export TSUtils.
utils/src/index.ts:7

prefer-includes (4 occurrences)

docs

Use .includes() instead of .some() when checking value existence.
eslint-plugin/src/rules/no-unnecessary-qualifier.ts:45
eslint-plugin/src/rules/padding-line-between-statements.ts:70
type-utils/src/TypeOrValueSpecifier.ts:126
type-utils/src/getTypeName.ts:57

prefer-math-trunc (4 occurrences)

docs

Use Math.trunc instead of << 0.
eslint-plugin/src/rules/naming-convention-utils/enums.ts:25
eslint-plugin/src/rules/naming-convention-utils/enums.ts:93
eslint-plugin/src/rules/prefer-regexp-exec.ts:17

Use Math.trunc instead of | 0.
website/src/components/linter/utils.ts:20

prefer-native-coercion-functions (2 occurrences)

docs

arrow function is equivalent to Boolean. Use Boolean directly.
typescript-estree/src/convert.ts:352
website/src/components/RulesTable/index.tsx:197

prefer-negative-index (1 occurrence)

docs

Prefer negative index over length minus index for at.
eslint-plugin/src/rules/no-unnecessary-parameter-property-assignment.ts:204

prefer-node-protocol (67 occurrences)

docs

Prefer node:child_process over child_process.
integration-tests/tools/pack-packages.ts:10
types/tools/copy-ast-spec.ts:1

Prefer node:fs over fs.
ast-spec/tests/fixtures.test.ts:1
eslint-plugin/tests/docs.test.ts:8
eslint-plugin/tests/index.test.ts:1
eslint-plugin/tests/rules/index.test.ts:1
integration-tests/tools/pack-packages.ts:11
parser/tests/lib/services.test.ts:2
repo-tools/src/generate-sponsors.mts:2
scope-manager/tests/fixtures.test.ts:1
types/tools/copy-ast-spec.ts:2
typescript-estree/src/create-program/getWatchProgramsForProjects.ts:2
typescript-estree/src/create-program/useProvidedPrograms.ts:2
typescript-estree/src/parseSettings/getProjectConfigFiles.ts:2
typescript-estree/tests/lib/persistentParse.test.ts:1
typescript-estree/tests/lib/semanticInfo.test.ts:1
website/plugins/generated-rule-docs/insertions/insertSpecialCaseOptions.ts:1
website/plugins/utils/rules.ts:6

Prefer node:os over os.
website/plugins/generated-rule-docs/insertions/insertNewRuleReferences.ts:4

Prefer node:util over util.
types/tools/copy-ast-spec.ts:4

prefer-regexp-test (3 occurrences)

docs

Prefer .test(…) over .exec(…).
eslint-plugin/src/util/getWrappingFixer.ts:70
parser/tests/test-utils/test-utils.ts:62
typescript-estree/tests/test-utils/test-utils.ts:47

prefer-spread (48 occurrences)

docs

Prefer the spread operator over Array#concat(…).
eslint-plugin/src/rules/consistent-type-exports.ts:293
eslint-plugin/src/rules/key-spacing.ts:417
eslint-plugin/src/rules/lines-around-comment.ts:159
eslint-plugin/src/rules/prefer-function-type.ts:113
eslint-plugin/tests/rules/no-inferrable-types.test.ts:14
eslint-plugin/tests/rules/type-annotation-spacing.test.ts:6569
eslint-plugin/tests/rules/type-annotation-spacing.test.ts:6611
rule-schema-to-typescript-types/src/printAST.ts:45
typescript-estree/src/convert.ts:1948
typescript-estree/src/convert.ts:2129
typescript-estree/src/create-program/getWatchProgramsForProjects.ts:338
typescript-estree/src/parseSettings/resolveProjectList.ts:98
typescript-estree/src/parseSettings/warnAboutTSVersion.ts:20
utils/src/eslint-utils/deepMerge.ts:23
website-eslint/src/mock/path.js:175

Prefer the spread operator over Array#slice().
eslint-plugin/src/rules/member-ordering.ts:508
eslint-plugin/src/rules/no-shadow.ts:651

Prefer the spread operator over String#split(``).
website/src/theme/NotFound/Content/index.tsx:16

prefer-string-replace-all (73 occurrences)

docs

prefer-string-slice (7 occurrences)

docs

Prefer String#slice() over String#substring().
eslint-plugin-internal/src/rules/plugin-test-formatting.ts:393
eslint-plugin/src/rules/method-signature-style.ts:76
eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts:404
eslint-plugin/tools/generate-breaking-changes.mts:34
rule-schema-to-typescript-types/src/index.ts:90
website-eslint/src/mock/assert.js:59
website/src/components/ast/PropertyValue.tsx:23

prefer-switch (5 occurrences)

docs

Use switch instead of multiple else-if.
eslint-plugin/src/rules/explicit-module-boundary-types.ts:185
eslint-plugin/src/rules/no-floating-promises.ts:297
eslint-plugin/src/rules/no-type-alias.ts:262
eslint-plugin/src/rules/return-await.ts:194
eslint-plugin/src/util/getThisExpression.ts:8

prefer-ternary (17 occurrences)

docs

prefer-top-level-await (10 occurrences)

docs

Prefer top-level await over an async IIFE.
repo-tools/src/postinstall.mts:31
website/plugins/generated-rule-docs/insertions/insertNewRuleReferences.ts:40

Prefer top-level await over using a promise chain.
eslint-plugin/tools/generate-breaking-changes.mts:159
repo-tools/src/generate-configs.mts:410
repo-tools/src/generate-contributors.mts:174
repo-tools/src/generate-lib.mts:310
repo-tools/src/generate-sponsors.mts:192
types/tools/copy-ast-spec.ts:86
website-eslint/build.ts:166
website/tools/generate-website-dts.ts:105

prefer-type-error (10 occurrences)

docs

new Error() is too unspecific for a type check. Use new TypeError() instead.
repo-tools/src/generate-contributors.mts:64
rule-tester/src/TestFramework.ts:112
rule-tester/src/TestFramework.ts:117
rule-tester/src/TestFramework.ts:167
rule-tester/src/TestFramework.ts:173
rule-tester/src/TestFramework.ts:205
rule-tester/src/TestFramework.ts:210
rule-tester/src/utils/config-validator.ts:122
rule-tester/src/utils/config-validator.ts:124
scope-manager/tests/fixtures.test.ts:103

text-encoding-identifier-case (6 occurrences)

docs

Prefer utf8 over utf-8.
eslint-plugin/tests/docs.test.ts:31
eslint-plugin/tests/docs.test.ts:537
integration-tests/tools/pack-packages.ts:45
types/tools/copy-ast-spec.ts:54
types/tools/copy-ast-spec.ts:61
typescript-estree/src/create-program/useProvidedPrograms.ts:77

Josh-Cena · 2024-07-23T14:50:16Z

I'm not familiar with eslint-plugin-unicorn so it took me slightly longer to look at some of the rules (in fact I'm surprised we have this plugin at all).

Tentatively, I'm +1 on (as in, I believe this coding style will benefit us):

catch-error-name (but we should either use e or err, not error—I actively hate full names)
custom-error-definition
error-message
no-console-spaces
no-for-loop (why isn't this caught by our prefer-for-of?)
no-useless-spread
prefer-add-event-listener
prefer-array-flat
prefer-array-some
prefer-at
prefer-code-point
prefer-dom-node-append
prefer-includes
prefer-math-trunc
prefer-negative-index
prefer-node-protocol
prefer-regexp-test
prefer-top-level-await
text-encoding-identifier-case

I'm +0/-0.5 on (as in, I believe this coding style is desirable, but it's either not useful or may take away some expressiveness unnecessarily):

better-regex (I'm a big fan of regexes and I think we should lint them with eslint-plugin-regexp (which I use a lot), which should be able to provide the same capabilities.)
no-array-push-push
no-lonely-if
no-unused-properties (not sure how reliable it is)
prefer-export-from
prefer-native-coercion-functions (this is a funny one because Boolean is not a predicate, but I'm open to testing because it looks like they aren't used as predicates anyway.)
prefer-spread
prefer-string-replace-all
prefer-string-slice
prefer-switch
prefer-ternary
prefer-type-error

I'm -1 on (as in, this actively goes against my coding preferences or may not work with other tools):

no-array-for-each
no-array-method-this-argument
no-array-reduce
no-hex-escape
no-object-as-default-parameter
no-useless-switch-case
no-useless-undefined
number-literal-case (this probably conflicts with Prettier, and is best useless)

sindresorhus · 2024-07-23T15:07:43Z

no-array-for-each

There are many benefit with for-of loops: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/no-array-for-each.md

no-object-as-default-parameter

This one is catching actual mistakes: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/no-object-as-default-parameter.md

Josh-Cena · 2024-07-23T15:21:58Z

no-array-for-each

I would prefer to leave this discretion to the author. The performance is not a concern in our case; readability is debatable, especially when the array expression is long; ability to skip iterations is also present with forEach, so the only real benefit seems to be ability to exit early, but that's rarely needed, if you look at the places it's reporting on.

no-object-as-default-parameter

The only place it's reporting on has only one property, if the rule's only motivation is "default options with more than one option". And in general I don't think that's a bug vector, since the type information would accurately tell us what each property can and cannot be.

abrahamguo · 2024-07-23T15:23:11Z

@Josh-Cena another benefit of prefer-for-each that might be more relevant for us:

Additionally, using for…of has great benefits if you are using TypeScript, because it does not cause a function boundary to be crossed. This means that type-narrowing earlier on in the current scope will work properly while inside of the loop (without having to re-type-narrow). Furthermore, any mutated variables inside of the loop will picked up on for the purposes of determining if a variable is being used.

abrahamguo · 2024-07-23T15:25:35Z

no-object-as-default-parameter

The only place it's reporting on has only one property

That's actually not correct; that file has

 options: VisitPatternOptions = { processRightHandNodes: false },

and earlier in that same file,

interface VisitPatternOptions extends PatternVisitorOptions {
  processRightHandNodes?: boolean;
}

and following the chain of interfaces, there are actually two more options:

interface VisitorOptions {
  childVisitorKeys?: VisitorKeys | null;
  visitChildrenEvenIfSelectorExists?: boolean;
}

Josh-Cena · 2024-07-23T15:28:34Z

I agree for...of loops have many benefits and I'm not against using it, but I don't think it's worthwhile to crystalize it in a rule.

Apologies, I misread the motivation for the no-object-as-default-parameter rule. This rule also prevents the case where someone passes an empty object and overrides the default which has one property. Still my point stands: this property is already typed to be nullable, so it doesn't matter if it's guaranteed to be present at runtime, since we have to write code to guard against this case anyway. If we change the type definition then TS will force the callsites to change too. I don't find it useful in a fully typed codebase.

JoshuaKGoldberg · 2024-07-23T18:06:00Z

(but we should either use e or err, not error—I actively hate full names)

I actively hate abbreviations and think we should use error, not e or err. 😂

I think we should lint them with eslint-plugin-regexp (which I use a lot), which should be able to provide the same capabilities

Strong +1. Potentially hot take (someone please yell at me if it's not good): any universally useful rules from eslint-plugin-unicorn that don't exist in eslint-plugin-regexp should IMO be filed as feature requests on eslint-plugin-regexp. IMO there's no need to have them in a catch-all general community plugin when a standard plugin for regexps exists.

abrahamguo · 2024-07-23T18:07:34Z

@JoshuaKGoldberg makes sense! and yes, the functionality from unicorn's one regexp rule can all be done by eslint-plugin-regexp.

abrahamguo · 2024-07-23T18:14:16Z

Responding to a few of @Josh-Cena's thoughts from earlier:

no-unused-properties (not sure how reliable it is)

At a quick glance at some of the reports, they certainly look correct. I use this rule at work and have not had any false positives from it. I'm guessing the rule is pretty conservative and bails out right away if the whole object is used somewhere.

prefer-native-coercion-functions (this is a funny one because Boolean is not a predicate, but I'm open to testing because it looks like they aren't used as predicates anyway.)

Yeah, inferred filter predicates just landed in TypeScript anyways, and you have to write your callbacks in very specific ways in order to use the inferred predicate feature. But certainly worth looking whether using real TypeScript predicates would be beneficial.

no-array-method-this-argument

Three of those reports are understable to keep that way. However, this one is certainly a mistake:

Do not use the this argument in Array#findIndex().

website/plugins/generated-rule-docs/insertions/insertWhenNotToUseIt.ts:18

no-array-reduce

Note that this rule does allow using reduce for array summations. I will want to look at the specific reports, because in at least some of them, it does seem like there is a more idiomatic method to use.

Josh-Cena · 2024-07-23T18:16:23Z

However, this one is certainly a mistake:

I'm surprised and I don't even remember what it's supposed to look like!

I will want to look at the specific reports, because in at least some of them, it does seem like there is a more idiomatic method to use.

Sounds good 👍 I'm also pro-remove-reduce, but want to allow the freedom in case it is useful.

kirkwaiblinger · 2024-07-24T07:01:18Z

@abrahamguo Just wanna say real quick I really like how neatly you've created the issue report and included the links and everything. Super helpful ❤️

I'll preface all of the following with saying that I haven't personally used any of these rules, so my opinions are only as good as my understanding of the rule's intent, my ignorance as to edge cases, and the limited amount of time I've spent thinking about each one....

rule	score	words
better-regex	+/- 0	others seem to know more
catch-error-name	+1	I significantly prefer unabbreviated `error` too
custom-error-definition	+1?	I don't understand this well, but it seems sensible. This report does seem like a false positive/bug in the rule though 🤔
error-message	+0.5	The one report is a special case we'd want to disable, but the rule seems reasonable
no-array-for-each	+1.5	I am a die-hard True Believer in `for...of`. I'll spare the manifesto for now, but I'll mention that I mostly disagree with the assessment in #9629 (comment); just use an intermediate variable.
no-array-method-this-argument	+1
no-array-push-push	-1	just not a fan
no-array-reduce	-0.5	Checked a few of the report examples, and they were heinous IMO. However, I think this is a concern that is the domain of human code review, not linting
no-console-spaces	+0.5
no-for-loop	+0.5	I like. But it's pretty opinionated.
no-hex-escape	+0.5?	I have no prior knowledge of this, but the rule description sounds sensible
no-lonely-if	-1 (edited)	~~how is this different from the eslint core rule?~~ This rule is even worse than the core rule. Booooo
no-object-as-default-parameter	+0.1	description sounds plausible, but haven't thought about this hard
no-unused-properties	-0.5?	This seems not useful when using TS. Note that the rule description even says, (paraphrasing) to overcome to the limitations of this rule, use TS.
no-useless-spread	+1
no-useless-switch-case	-0.1	It's useful to be explicit sometimes.... A glance at the reports didn't seem too suspicious.
no-useless-undefined	-100	Actively, strongly disagree with the premise of this rule (particularly in the context of TS)
number-literal-case	-1 (edited)	~~Does other tooling (i.e. prettier) already handle this?~~ Conflicts with other tooling
prefer-add-event-listener	+0.5?
prefer-array-flat	+1
prefer-array-some	-0.5?	See also #8378 🤔
prefer-at	-0.5?	See also #6401 🤔
prefer-code-point	+/- 0	can't be bothered to read up on this one
prefer-dom-node-append	+/-0	🤷
prefer-export-from	+0.5
prefer-includes	-1	we should be dogfooding https://typescript-eslint.io/rules/prefer-includes/ instead
prefer-math-trunc	+0.5	3/4 cases are `1 << 0` in a flags enum, which should stay as-is. The remaining 1 case looks like an actual bug
prefer-native-coercion-functions	+/-0	don't have the attention span to think hard about this one right now, but I'm wary of it (and no-implicit-coercion is my main concern anyway)
prefer-negative-index	-1?	related to prefer-at, see #6401 🤔
prefer-node-protocol	+0.1
prefer-regexp-test	+0.1
prefer-spread	+0.5
prefer-string-replace-all	+0.1
prefer-string-slice	+0.5
prefer-switch	-1
prefer-ternary	-0.5
prefer-top-level-await	+1	This one is quite intriguing to me, and may be helpful to keep in mind for users with pains around no-floating-promises
prefer-type-error	-0.5	This feels like a JS practice for validation that we avoid having to do in TS due to the type system, but I could be wrong
text-encoding-identifier-case	+/-0?	does TS type system itself not enforce/encourage this?

EDIT - Added second batch of rules

Josh-Cena · 2024-07-24T07:09:58Z

just use an intermediate variable.

Needless to say my biggest principle is concise code, so "no" to full names, "no" to redundant braces, and "no" to intermediate variables, but I can see how I'm in the minority especially among linter maintainers :P

how is this different from the eslint core rule?

The core rule only reports on else { if {} }. This one reports on if { if {} }, which I personally think is fine.

Does other tooling (i.e. prettier) already handle this?

Yes.

kirkwaiblinger · 2024-07-24T07:20:31Z

Needless to say my biggest principle is concise code, so "no" to full names, "no" to redundant braces, and "no" to intermediate variables, but I can see how I'm in the minority especially among linter maintainers :P

Most concise != least entropy 😁

how is this different from the eslint core rule?

The core rule only reports on else { if {} }. This one reports on if { if {} }, which I personally think is fine.

Gotcha. I'm -1 on both 👍

Does other tooling (i.e. prettier) already handle this?

Yes.

Cool, sounds like we should omit it then 👍

kirkwaiblinger · 2024-07-24T07:52:35Z

Throwing this out there, I think we should automatically reject adding any rule from unicorn that has a near-identical form in typescript-eslint, or has a proposal in typescript-eslint that is "accepting PRs".

If it exists in our plugin, we should use it, and if it's inadequate, we should fix it.

If it doesn't yet exist, but we have accepted it, it either means

We're aware of the unicorn version, but believe that it's lacking in functionality without type information for some reason.
Or, we weren't aware of the unicorn version. In that case, and if it's adequate without type information, then we should close/"wontfix" the proposal in typescript-eslint, and use the unicorn version. Otherwise, we should just dogfood our own version once it's released.

I've noted one or two rules with near-identical forms or proposals thereof in typescript-eslint, but there may be more. Let's be sure to double check before proceeding with the unicorn rules.

JoshuaKGoldberg · 2024-07-25T12:49:54Z

@meszaros-lajos-gyorgy you're right: that is off topic and not what this issue is about 🙂. https://typescript-eslint.io/contributing/issues#questions-and-support-requests has links to places you can ask for help.

fisker · 2024-07-27T12:20:49Z

No one likes prefer-string-raw?

Josh-Cena · 2024-07-27T14:12:56Z

It depends. String.raw is 10 characters long so the rule is only beneficial if there are more than 10 backslashes in the string.

bradzacher · 2024-07-27T14:14:20Z

so the rule is only beneficial if there are more than 10 backslashes in the string

I don't think the point of String.raw is conciseness 😅
It's about readability - you don't need to think about escaping backslashes so it's easier to read and understand such strings.

eg "C:\\windows\\style\\path\\to\\file.js" is less clear than String.raw`C:\windows\style\path\to\file.js`

Josh-Cena · 2024-07-27T14:27:19Z

I find them equally readable, FWIW, especially with proper syntax highlighting that tells you what is escaped and what is not. I'm not strongly opposed to this rule though.

bradzacher · 2024-07-27T14:58:48Z

rule	score	words
better-regex	+1
catch-error-name	-1	I personally dislike restrictions on names -- I think that they are bikeshedding stylistic rules that don't actually improve readability
custom-error-definition	+1
error-message	0	I don't think it actually will catch a bug for us
no-array-for-each	0	I'm in favour of "standardising the codebase", but our codebase has about 50/50 `forEach` vs `for..of`. IDK I guess we could standardise on `for..of`?
no-array-method-this-argument	+1
no-array-push-push	-1e9999	Many of the reports I very purposely wrote like that because it causes the code to be laid out in an easy to read fashion. I don't think this nitpick is valuable, personally -- seems weird to be in their recommended set.
no-array-reduce	-1e9999	`reduce` has its uses and banning it outright isn't good. I'm gobsmacked that such an opinionated rule is in the recommended set.
no-console-spaces	+1
no-for-loop	-1	The idea is great but the implementation seems wrong. For example this loop uses its `i` for things other than array index, or both this loop and this loop uses the `i` for accessing two different arrays at once.
no-hex-escape	-1	IDK if one is clearer than the other, IMO. And the one case it's reporting on is clearer with hex codes cos it's console colour escapes.
no-lonely-if	+1
no-object-as-default-parameter	0	Sure I guess but IDC either way
no-unused-properties	+1
no-useless-spread	+1
no-useless-switch-case	-1e9999	I prefer being explicitly exhaustive and listing conditions out to make it clear that they were considered and handled as opposed to leaving it implicit.
no-useless-undefined	-1	I don't think this makes code any better - an explicit `undefined` can make code easier to understand
number-literal-case	0	I don't think this makes anything clearer, personally but whatever
prefer-add-event-listener	0	We're not a webapp mostly and we don't use event listeners - I don't think it's worth turning on
prefer-array-flat	+0.5
prefer-array-some	0	Meh IDC
prefer-at	+1
prefer-code-point	-1	Just cos the only reports are from code that was synced from TS
prefer-dom-node-append	0	Ditto for add event listener
prefer-export-from	+0.5
prefer-includes	+1
prefer-math-trunc	-1	cos it's flagging `<< 0` which we commonly use in flag enums for the first member
prefer-native-coercion-functions	+1
prefer-negative-index	+1
prefer-node-protocol	+1
prefer-regexp-test	+1
prefer-spread	-1	I don't think that `[...x]` is clearer or better than `Array.from(x)`
prefer-string-replace-all	+1
prefer-string-slice	0	Almost a -1 just cos of the rule's description "String#substr() and String#substring() are the two lesser known legacy ways to slice a string" -- like "lesser known"? WHAT?
prefer-switch	-1	I don't think a switch is always the better construct -- I think that an if/else can make code clearer sometimes.
prefer-ternary	-1	I don't think conciseness is better or clearer. And looking at some of those reports it would actively harm the code clarity.
prefer-top-level-await	-♾	We don't use MJS so we can't use TLA always. Also this case should NOT use TLA because the `await` is in a nested scope. So the rule is wrong.
prefer-type-error	-1e9999	It's misclassifying many of our conditions as "type check conditions". So it's just wrong?
text-encoding-identifier-case	+1

Josh-Cena · 2024-07-27T15:14:25Z

better-regex +1

This rule may become deprecated and is certainly buggy: sindresorhus/eslint-plugin-unicorn#2409 and better alternatives exist (as proposed above), so I hope you are not too married to it.

no-array-reduce -1e9999

Could you give a valid reduce use case that cannot, or should not, be refactored to a for...of loop for better performance and readability? This would be helpful to be added to the MDN docs too, since I scratched my head to come up with two (2) valid use cases: summation (and other related operations), function piping (and its async version).

no-for-loop -1

The suggestion is to use entries(). I don't have strong opinions but I slightly prefer C-style for loops.

prefer-add-event-listener 0

We have a web app: the website.

fisker · 2024-07-27T15:15:12Z

Also this case should NOT use TLA because the await is in a nested scope. So the rule is wrong.

You misunderstood, it reports not because there is an await, but because prettierConfig is a Promise

bradzacher · 2024-07-27T22:15:11Z

We have a web app: the website.

Sure but it's react - so we don't delve into the imperative APIs much. And it's also minimal amounts of code. Hence "pretty low value for our codebase".

The suggestion is to use entries().

I don't see how

for (const [i, paramA] of Object.entries(paramsA)) {
  const paramB = paramsB[i];

Is in any way clearer than

for (let i = 0; i < paramsA.length; ++i) {
  const paramA = paramsA[i];
  const paramB = paramsB[i];

It's worse in every way, IMO.

So will update my vote to -1e9999

Could you give a valid reduce use case that cannot, or should not, be refactored to a for...of loop for better performance and readability?

It's an opinionated stylistic thing to choose one over the other. In your opinion a for loop is more readable.

For example I disagree that

const result = {} as ParsedOptions;
for (const k of getEnumNames(Selectors)) {
  result[k] = createValidator(k, context, normalizedOptions); 
}

Is necessarily clearer than

const result = getEnumNames(Selectors).reduce((acc, k) => {
  acc[k] = createValidator(k, context, normalizedOptions);
  return acc;
}, {} as ParsedOptions);

Being functional, reduce promotes isolation and encapsulation - it makes you think "I don't want to mutate values outside of the loop closure" - which is good for minimising side-effects of the loop and making code more predictable.

But the for..of version is made to explicitly mutate a value outside the loop closure. It purposely invites you to add side-effects to loops.

I'm firmly against this rule. I understand some people don't like reduce - but it's ultimately an opinionated, stylistic choice to use one over the other.
Some people are very loudly and vehemently against using enums but that doesn't mean they're right or that everyone should migrate off of them. I see this as the same class of opinionated, stylistic debate that codebases can choose to lint for if they want.

better-regex +1
This rule may become deprecated

🤷‍♂️ I didn't look into it too much. I noticed you guys were discussing it above. I was just weighing in that I'm good with the idea of linting to improve our regex.

alfaproject · 2024-08-04T06:43:00Z

I accidentally stumbled upon this thread while checking the stylistic typed recommended rules. We also use unicorn and some of the rules look nearly identical but in the documentation they are not mentioned so it's not obvious to me whether I should keep using the unicorn ones that look identical since they don't need types, or I should instead prefer the ones from here.

I see above that there is awareness of the overlap, but have you considered adding to the docs why the ones in this repo are better (nor not) than the ones from unicorn?

bradzacher · 2024-08-04T08:42:15Z

Often times the overlap is that the two projects grew independently and there weren't open lines of communication to keep track of what each other was working on.

Sometimes it because we offered a type-aware version (more correct but slower and limited to TS codebases) and they chose a pure-syntactic version (faster, chance of false positives, applies to non-TS codebases).

Which rules are you talking about specifically?

alfaproject · 2024-08-04T10:38:46Z

Some examples:

There's probably more, I only had a quick look at the stylish ones like I mentioned.

I understand that both projects grow independently but I was just wondering if it would be helpful to add in the documentation like you do for base eslint rules, something like, if you enable this rule, please bear in mind that it might conflict with this other rule from unicorn (or some other popular plugin).

In terms of documentation I've seen other plugin authors doing the same for popular plugins which I think unicorn qualifies. This can be done by the community ofc if that's desired. For instance in this plugin: https://github.com/lydell/eslint-plugin-simple-import-sort

Make sure not to use other sorting rules at the same time:

sort-imports

import/order

JoshuaKGoldberg · 2024-10-17T16:53:22Z

@abrahamguo were there any other rules you wanted to enable here? Just checking before we close this out. 🙂

abrahamguo · 2024-10-18T18:41:38Z

Should be good to close!

abrahamguo added repo maintenance things to do with maintenance of the repo, and not with code/docs triage Waiting for team members to take a look labels Jul 23, 2024

This was referenced Jul 24, 2024

chore: enable unicorn/no-for-loop #9628

Closed

chore: enable unicorn/no-array-for-each #9629

Closed

This comment was marked as off-topic.

Sign in to view

abrahamguo mentioned this issue Jul 25, 2024

chore: enable unicorn/no-array-reduce #9640

Merged

3 tasks

This was referenced Jul 29, 2024

chore: enable prefer-regexp-test #9658

Merged

chore: enable unicorn/no-useless-spread #9687

Merged

abrahamguo mentioned this issue Aug 7, 2024

chore: enable unicorn/prefer-node-protocol #9742

Merged

3 tasks

typescript-eslint deleted a comment Aug 9, 2024

JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Aug 22, 2024

This was referenced Aug 27, 2024

chore: enable unicorn/prefer-ternary #9886

Closed

chore: enable unicorn/prefer-native-coercion-functions #9909

Closed

chore: enable unicorn/prefer-structured-clone #9911

Merged

This was referenced Sep 3, 2024

chore: enable unicorn/no-length-as-slice-end #9915

Merged

chore: enable unicorn/no-single-promise-in-promise-methods #9916

Merged

bradzacher closed this as completed in #9916 Sep 3, 2024

abrahamguo mentioned this issue Sep 4, 2024

chore: enable unicorn/prefer-array-some #9932

Merged

3 tasks

JoshuaKGoldberg reopened this Sep 4, 2024

abrahamguo mentioned this issue Sep 6, 2024

chore: enable eslint-plugin-regexp #9948

Merged

3 tasks

abrahamguo closed this as completed Oct 18, 2024

github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Oct 26, 2024

github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Repo: enable more `eslint-plugin-unicorn` rules internally #9623

Repo: enable more `eslint-plugin-unicorn` rules internally #9623

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Repo: enable more eslint-plugin-unicorn rules internally #9623

Repo: enable more eslint-plugin-unicorn rules internally #9623

Comments

Suggestion

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Repo: enable more `eslint-plugin-unicorn` rules internally #9623

Repo: enable more `eslint-plugin-unicorn` rules internally #9623