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

Add base ESLint config #3

Merged
merged 6 commits into from
Apr 3, 2023
Merged

Add base ESLint config #3

merged 6 commits into from
Apr 3, 2023

Conversation

mcous
Copy link
Member

@mcous mcous commented Mar 30, 2023

This PR adds base ESLint config. I didn't add any rules, just base extensions. Could use help selecting more rules for this PR!

Copy link
Member

@DTCurrie DTCurrie left a comment

Choose a reason for hiding this comment

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

✅ pending decision on explicit rule setting

@mcous
Copy link
Member Author

mcous commented Mar 31, 2023

I did my best to incorporate the app's ESLint config into the base config here. Intentional deviations from the app's explicit and/or effective config:

Things that were explicitly errors and are now off

Things that were implicitly errors via eslint:all and are now off

  • block-scoped-vars - Unnecessary because we don't allow var
  • complexity - Redundant with sonarjs/cognitive-complexity
  • consistent-this - We have arrow functions now
  • dot-notation - Replaced by @typescript-eslint/dot-notation
  • func-name-matching - We have arrow functions now
  • id-denylist - Needs an explicit configuration to do anything
  • id-match - Needs an explicit configuration to do anything
  • no-array-constructor - Redundant with @typescript-eslint/no-array-constructor
  • no-empty-function - Redundant with @typescript-eslint/no-empty-function
  • no-eq-null - Redundant with eqeqeq
  • no-implied-eval - Replaced by @typescript-eslint/no-implied-eval
  • no-inline-comments - Redundant with line-comment-position
  • no-loss-of-precision - Redundant with @typescript-eslint/no-loss-of-precision
  • no-restricted-* - Needs an explicit configuration to do anything
  • no-throw-literal - Replaced by @typescript-eslint/no-throw-literal
  • no-useless-constructor - Replaced by @typescript-eslint/no-useless-constructor
  • sort-vars - We don't only multiple variables initialized in one line

Things that were off and are now errors

  • no-undef - Enabled by eslint:recommended
    • According to comments, it was disabled in app due to issues with Vue.
    • I'd like to keep it enabled in the base config and disable in a Vue-specific config, if necessary.
  • no-unused-var - Replaced by @typescript-eslint/no-unused-vars, enabled by recommended config
    • Same story
  • unicorn/no-empty-file - Enabled by unicorn/recommended and also recommended by me.
    • If we really need an empty file, we can add an override or a comment.
  • unicorn/filename-case - Enabled by unicorn/recommended and also recommended by me.
    • I've seen too many people get tripped up by case-sensitive vs case-insensitive filesystems working in JS codebases over the years
    • This rule with its default of kebab-case.ext is good
    • viamrobotics/prime had two inconsistently cased filenames that I saw (and fixed) last week

Things that are no longer explicit, but still enabled from extended recommendations.

  • no-unsafe-optional-chaining
  • unicorn/import-style
  • @typescript-eslint/no-explicit-any
  • @typescript-eslint/ban-ts-comment

Other notes

  • I extended plugin:@typescript-eslint/strict, which means several rules that were not specified one way or another are now on
  • I did not extend and vue rules because I'd like to layer on a Vue-specific config rather than incorporate it into the base.
  • Ditto for tailwind
  • If a rule is marked as off in the app's config and not mentioned here nor in the base config, it is still off in the effective config, either due to extending a config or by omission (unless I missed something!)

Copy link
Member

@micheal-parks micheal-parks left a comment

Choose a reason for hiding this comment

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

Great stuff here, thank you!

@mcous mcous merged commit f682aff into viamrobotics:main Apr 3, 2023
@mcous mcous deleted the add-eslint-config branch April 3, 2023 18:19
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.

4 participants