Investigate removing eslint rules - Githubissues

button-inc / digital_marketplace

The intent of this development is to build a product that will support digital procurement needs for the BC Government including services such as, but not limited to, Sprint With Us, Code With Us, The Procurement Concierge.

Apache License 2.0

0 stars 0 forks source link

Investigate removing eslint rules #25

Closed BryanGK closed 2 years ago

BryanGK commented 2 years ago

ref #13

As a developer, I want to remove prioritized eslint rules that expose possible problems in the code to be fixed, so that we can begin to bolster the health of the codebase.

DoD:

prioritized list of rules to be removed in order of importance
a plan of how to fix the errors
estimated time to fix each error

wenzowski commented 2 years ago

In particular, let's prioritize these in order of importance. Rules that expose possible problems should likely be prioritized before suggestions, and layout/formatting rules may not ever be necessary to change.

"@typescript-eslint/no-explicit-any": "off",
"react/prop-types": "off",
"@typescript-eslint/no-unused-vars": "off",
"no-useless-escape": "off",
"react/no-unescaped-entities": "off",
"no-case-declarations": "off",
"no-undef": "off",
"no-fallthrough": "off",
"@typescript-eslint/no-var-requires": "off",
"@typescript-eslint/no-empty-function": "off",
"@typescript-eslint/ban-types": "off",
"react/display-name": "off",
"react/no-children-prop": "off",
"no-duplicate-case": "off",
"react/jsx-key": "off",
"@typescript-eslint/no-non-null-assertion": "off",
"react/jsx-no-target-blank": "off"

BCerki commented 2 years ago

Possible Problems
"no-undef": "off"--disallow the use of undeclared variables unless mentioned in `/*global */` comments
"no-fallthrough": "off",--disallow fallthrough of `case` statements
"no-duplicate-case": "off",--disallow duplicate case labels

Suggestions
"no-useless-escape": "off",--disallow unnecessary escape characters
"no-case-declarations": "off",--disallow lexical declarations in case clauses

Typescript
Recommended
"@typescript-eslint/no-explicit-any": "off",
"@typescript-eslint/no-unused-vars": "off",
"@typescript-eslint/no-non-null-assertion": "off",
"@typescript-eslint/no-var-requires": "off",
"@typescript-eslint/no-empty-function": "off",
"@typescript-eslint/ban-types": "off",

React
Recommended
"react/display-name": "off",
"react/no-children-prop": "off",
"react/jsx-key": "off",
"react/prop-types": "off",
"react/no-unescaped-entities": "off",
"react/jsx-no-target-blank": "off"

Typescript rules: https://github.com/typescript-eslint/typescript-eslint/tree/main/packages/eslint-plugin React rules: https://github.com/yannickcr/eslint-plugin-react

wenzowski commented 2 years ago

I'd consider the next step to be to set the following so we can all see transparently on CI what we're dealing with.

"no-undef": "warn"
"no-fallthrough": "warn",
"no-duplicate-case": "warn",

BCerki commented 2 years ago

I've also set these two to warn to allow the example Cypress tests to pass CI:

"cypress/no-unnecessary-waiting": "warn",
 "@typescript-eslint/ban-ts-comment": "warn"

And a few more:

"mocha/no-mocha-arrows": "warn",
"mocha/max-top-level-suites": "warn",
"mocha/no-sibling-hooks": "warn"

BCerki commented 2 years ago

I've also set these two to warn to allow the example Cypress tests to pass CI:
"cypress/no-unnecessary-waiting": "warn",
 "@typescript-eslint/ban-ts-comment": "warn"
And a few more:
"mocha/no-mocha-arrows": "warn",
"mocha/max-top-level-suites": "warn",
"mocha/no-sibling-hooks": "warn"

Turned these back to error--decided to delete Cypress example tests per https://github.com/button-inc/digital_marketplace/pull/55

BCerki commented 2 years ago

Remaining eslint errors:

react/no-children-prop. Looks very time-consuming to fix: https://github.com/button-inc/digital_marketplace/pull/66/files#diff-16f4da1941196a692e23a4fc30440dcd6148e133f4e009ec18646b5f2e821a0aR69-R70
react/prop-types. I think~ we can leave this one off because we're already using TS to check props: https://github.com/yannickcr/eslint-plugin-react/issues/1966
@typescript-eslint/no-explicit-any--Very time-consuming to fix, this rule prevents the use of any, which is all over the codebase
@typescript-eslint/no-unused-vars: "off". This only gives warnings, to address in #86
@typescript-eslint/no-non-null-assertion: "off". This only gives warnings, to address in #87
@typescript-eslint/no-empty-function. This turns up 2 warnings, 2 errors, and the erroring lines have // tslint:disable-next-line: no-empty attached so the code was clearly intentional. (Maybe for stub for testing?)

BCerki commented 2 years ago

Created tickets for outstanding rules as tech debt