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

Re-Enable ESLlint rules #11691

Closed
jhonnyoddball opened this issue Jul 27, 2020 · 11 comments
Closed

Re-Enable ESLlint rules #11691

jhonnyoddball opened this issue Jul 27, 2020 · 11 comments
Labels
tools-fe Used for the frontend tools team

Comments

@jhonnyoddball
Copy link
Contributor

To be able to upgrade ESLint to v. 7, some rules needed to be diasbled to complete the upgrade.

These rules need to be enabled again:

/* ----- TODO: evaluate and potentially turn these rules back on ----- */

    /* TODO | DISABLED TEMPORARILY */
    "max-classes-per-file": 0, //1
    "no-buffer-constructor": 0, //13
    "prefer-promise-reject-errors": 0, //14
    "func-names": 0, //14
    "no-restricted-globals": 0, //28
    "no-else-return": 0, //41 (fixable)
    "no-unused-vars": 0, //61
    "prefer-object-spread": 0, //78 (fixable)
    "lines-between-class-members": 0, //103 (fixable)
    "prefer-destructuring": 0, //261 (fixable)

    "react/jsx-fragments": 0, //2 (fixable)
    "react/jsx-closing-tag-location": 0, //3 (fixable)
    "react/no-typos": 0, //5
    "react/state-in-constructor": 0, //7
    "react/no-unused-state": 0, //15
    "react/sort-comp": 0, //23
    "react/default-props-match-prop-types": 0, //44
    "react/static-property-placement": 0, //50
    "react/jsx-wrap-multilines": 0, //60 (fixable)
    "react/jsx-curly-brace-presence": 0, //71 (fixable)
    "react/no-access-state-in-setstate": 0, //79
    "react/jsx-curly-newline": 0, //95 (fixable)
    "react/button-has-type": 0, //196
    "react/jsx-props-no-spreading": 0, //767
    "react/jsx-one-expression-per-line": 0, //2188 (fixable)
    "react/destructuring-assignment": 0, //3010

    "jsx-a11y/control-has-associated-label": 0, //7
    "jsx-a11y/click-events-have-key-events": 0, //10
    "jsx-a11y/anchor-is-valid": 0, //19
    "jsx-a11y/label-has-associated-control": 0, //35
    "jsx-a11y/label-has-for": 0, //66

    "import/named": 0, //2
    "import/no-useless-path-segments": 0, //59  (fixable)
    "import/no-cycle": 0, //105
    "import/order": 0, //544 (fixable)
@jhonnyoddball jhonnyoddball added the tools-fe Used for the frontend tools team label Jul 27, 2020
@mchelen-gov
Copy link
Contributor

mchelen-gov commented Aug 12, 2020

➕ 1️⃣ for enabling no-unused-vars
i'm guessing the number after the // is the number of errors from that rule?

@jhonnyoddball
Copy link
Contributor Author

➕ 1️⃣ for enabling no-unused-vars
i'm guessing the number after the // is the number of errors from that rule?

That is correct!

@mpelzsherman
Copy link
Contributor

Is it necessary for ALL of these rules to be re-enabled in one go? Or can we break this up into smaller chunks?

I'm happy to lend a hand if you need help with this. I'm particularly missing no-unused-vars.

@jhonnyoddball
Copy link
Contributor Author

Is it necessary for ALL of these rules to be re-enabled in one go? Or can we break this up into smaller chunks?

Well, I usually take 1 at the time. In addition, I have to group the errors by code owners and create multiple PRs for each code owner per rule.

I'm happy to lend a hand if you need help with this. I'm particularly missing no-unused-vars.

Yeah, absolutely!

cc: @meganhkelley

@bkjohnson
Copy link
Contributor

As an update, I'm working on fixing the issues around no-unused-vars right now.

@mchelen-gov
Copy link
Contributor

@bkjohnson would it be possible to set the { "args": "all" } for no-unused-vars https://eslint.org/docs/rules/no-unused-vars#args ?
with the desired behavior that any unused args (regardless of position) should be prefixed with _

@bkjohnson
Copy link
Contributor

We're unlikely to enforce that kind of behavior any time soon. If specific app teams want to adopt that style among themselves, then that's fine.

@meganhkelley meganhkelley added qa-standards Quality Assurance Standards associated work items and removed qa-standards Quality Assurance Standards associated work items labels Sep 2, 2021
@timwright12
Copy link
Contributor

I'm going to start chipping away at this one

@timwright12
Copy link
Contributor

Working on the jsx/a11y linting rules first. Initial run turned up 231 errors. I'm fixing small ones and reaching out to teams for big issues that require more context to fix. Will likely turn these on as warnings until they're cleared out.

@bkjohnson
Copy link
Contributor

bkjohnson commented Jan 25, 2022

I don't know if the issues are distributed conveniently enough for this to work without a lot of tedious effort, but we could use overrides to selectively turn those warnings into errors for certain directories. This could prevent them from popping up anywhere where they didn't already exist.

@timwright12
Copy link
Contributor

Opened up a PR that should only turn on jsx-a11y linting error on changed files, so if someone is in there making changes they'll have to also fix the issues:

department-of-veterans-affairs/vets-website#20034

We can use this as a mechanism for the rest as well moving forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools-fe Used for the frontend tools team
Projects
None yet
Development

No branches or pull requests

6 participants