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

feat: use recommended unicorn config #2502

Merged
merged 2 commits into from
Feb 21, 2025

Conversation

aaneitchik
Copy link
Contributor

To get the most out of the ESLint plugins we include, it's better if we extend recommended rules so that we get useful updates whenever they're extended. I reviewed our rule overrides to check which have the same configuration in the recommended config and removed these, left a few that are configured differently.

'unicorn/no-for-loop': 'error',
'unicorn/no-hex-escape': 'error',
'unicorn/no-instanceof-builtins': 'error',
'unicorn/no-keyword-prefix': 'off',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is off in the recommended config as well

// This is covered by @typescript-eslint/no-deprecated
'unicorn/no-new-buffer': 'off',
'unicorn/no-null': 'off',
// Unnecessary with TypeScript.
'unicorn/no-object-as-default-parameter': 'off',
'unicorn/no-process-exit': 'off',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this looks useful to me, and there's not that many usages across our codebases, so I think it's fine if it's disabled on a case-by-case basis

// This is covered by @typescript-eslint/no-deprecated
'unicorn/no-new-buffer': 'off',
'unicorn/no-null': 'off',
// Unnecessary with TypeScript.
'unicorn/no-object-as-default-parameter': 'off',
'unicorn/no-process-exit': 'off',
// Do not use classes in the first place.
'unicorn/no-static-only-class': 'off',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this also seems like it would be useful if for some reason the codebase allows classes, so removed this override

'unicorn/no-unreadable-array-destructuring': 'off',
'unicorn/no-unused-properties': 'off',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is also off in the recommended config

'unicorn/prefer-date-now': 'error',
'unicorn/prefer-default-parameters': 'error',
// Manual DOM manipulation is discouraged.
'unicorn/prefer-dom-node-append': 'off',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why turn these off, like if we do manipulate the DOM for some reason, why not have these suggestions

'unicorn/prefer-number-properties': 'error',
'unicorn/prefer-object-from-entries': 'error',
// Not supported yet.
'unicorn/prefer-object-has-own': 'off',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is now part of ESLint rules, not unicorn

'unicorn/require-array-join-separator': 'error',
'unicorn/require-number-to-fixed-digits-argument': 'error',
// No good use cases yet.
'unicorn/string-content': 'off',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

also off in the recommended config

// No good use cases yet.
'unicorn/string-content': 'off',
'unicorn/throw-new-error': 'error',
'unicorn/switch-case-braces': ['error', 'avoid'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the default is always require braces, and from looking at our codebases it seems like it's not how we usually write it. changed to avoid for this reason, see docs for details

@aaneitchik aaneitchik marked this pull request as ready for review February 21, 2025 10:28
@aaneitchik aaneitchik requested a review from a team as a code owner February 21, 2025 10:28
@reyawn
Copy link
Contributor

reyawn commented Feb 21, 2025

Nice!!

@aaneitchik aaneitchik merged commit b1dc200 into master Feb 21, 2025
4 checks passed
@aaneitchik aaneitchik deleted the feat/use-recommended-unicorn-config branch February 21, 2025 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants