Framework: Define all ESLint rules as errors

[aduth]

We use ESLint to enforce a consistent code standard. Over time, we have changed and added rules to our configuration. In doing so, because legacy code often violated the latest configuration settings, they could not be set to throw errors during continuous integration. As background, ESLint allows you to configure a rule with as a warning (1) or error (2), the latter being of higher severity and failing the continuous integration build. Enforcing our configuration during the continuous integration build step (and pre-commit hooks) helps highlight issues programmatically rather than through human intervention (i.e. peer review).

For this reason, we should seek to upgrade all rules in our ESLint configuration to be defined as errors. Because we extend the eslint-config-wpcalypso shared configuration, the process for doing this is to simply remove our warning overrides from the configuration. As such, rules defined as warnings in our ESLint configuration should effectively serve as a checklist of remaining items to be resolved in the codebase.

[aduth]

@nosolosw Is this issue no longer relevant given the merging of #6945 ?

[oandregal]

Some rules still need fixing, so I believe this is still relevant as a placeholder. It is a good time to re-purpose the goal and change the title and description, though.

One thing I'd like us to do is using ESLint at its full potential: making use of its --fix abilities to reformat our code on the fly directly in the editor. For me, it has worked well for all new files I've written, but I couldn't enable it for older ones as I didn't want to mess with legacy code and make my PRs less readable for the reviewers. That's a blocker we could remove.

I'd suggest focusing the efforts of this task in solving the fixable rules first. From that on, my understanding is that we can have ESLint always activated.

These are the fixable rules in client, server and test directories:

• array-bracket-spacing, 58 issues - Framework: Upgrade ESLint array-bracket-spacing to error #11425
• brace-style, 27 issues - Framework: Upgrade ESLint brace-style to error #11426
• comma-spacing, 4 issues - Framework: Upgrade ESLint comma-spacing to error #11427
• computed-property-spacing, 673 issues
• curly, 5 issues - Framework: Upgrade ESLint curly to error #11429
• func-call-spacing, 10 issues
• indent, 89 issues
• jsx-quotes, 32 issues
• key-spacing, 5 issues
• keyword-spacing, 23 issues
• no-else-return, 36 issues
• no-extra-semi, 21 issues
• no-multi-spaces, 15 issues
• no-multiple-empty-lines, 38 issues
• no-trailing-spaces, 9 issues
• no-var, 2402 issues
• object-curly-spacing, 18 issues
• operator-linebreak, 66 issues
• padded-blocks, 83 issues
• prefer-const, 255 issues
• quote-props, 352 issues
• quotes, 91 issues
• semi, 186 issues
• semi-spacing, 1 issues
• space-before-blocks, 5 issues
• space-before-function-paren, 37 issues
• space-in-parens, 165 issues
• space-infix-ops, 14 issues
• space-unary-ops, 31 issues
• template-curly-spacing, 135 issues

@aduth what do you think?

[aduth]

@nosolosw This sounds like a good idea. How do you suggest a person generate a report to tell them which files are problematic? Running the raw ./node_modules/.bin/eslint . ?

[Copons]

Just came back from a bunch of AFK days to find all my ESLint upgrades PR reviewed by @aduth! 🙇
As soon as I rebase and merge all of them I'll update @nosolosw's checklist and keep on fixin' the others.

[oandregal]

How about creating npm run lint-fixable to report the fixable rules? Early work for this in #12517

[stale]

This issue has been marked as stale because it hasn't been updated in a while. It will be closed in a week.
If you would like it to remain open, can you please you comment below and see what you can do to get things moving with this issue?
Thanks! 🙏