Skip to content

Restore ESLint warnings as errors (ignoring them for now) - #4382

Merged
mcmire merged 6 commits into
mainfrom
remove-eslint-warnings
Jun 10, 2024
Merged

Restore ESLint warnings as errors (ignoring them for now)#4382
mcmire merged 6 commits into
mainfrom
remove-eslint-warnings

Conversation

@mcmire

@mcmiremcmire commented Jun 6, 2024

Copy link
Copy Markdown
Collaborator

Explanation

Currently, yarn lint produces a bunch of warnings from ESLint. These warnings were added in a previous commit that upgraded ESLint packages as a way to avoid fixing lint violations created by the upgrade. However, this causes two problems:

  1. They produce a lot of noise that makes it very difficult to find true errors, especially when looking at a CI run.
  2. They allow new instances of these violations to show up (because warnings don't cause eslint to fail).

This commit removes the overrides from the ESLint config for these rules so that they go back to being errors and adds eslint-disable directives above the lines that cause the violations. Note that this is not intended to be a long-term solution, and in the future, we should dedicate time to either fixing the violations or explaining why ignoring them is necessary.

References

Fixes#4381.

Changelog

(There are no changes to functionality.)

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@mcmire
mcmire requested a review from a team as a code ownerJune 6, 2024 19:58
@mcmire
mcmire requested a review from a teamJune 6, 2024 19:58
@mcmire
mcmireforce-pushed the remove-eslint-warnings branch from d025d55 to 3c84ac6CompareJune 6, 2024 20:00
@mcmire
mcmire requested a review from a teamJune 6, 2024 20:01
Currently, `yarn lint` produces a bunch of warnings from ESLint. These
warnings were added in a previous commit that upgraded ESLint packages
as a way to avoid fixing lint violations created by the upgrade.
However, this causes two problems:
1. They produce a lot of noise that makes it very difficult to find true
errors, especially when looking at a CI run.
2. They allow new instances of these violations to show up (because
warnings don't cause `eslint` to fail).
This commit removes the overrides from the ESLint config for these rules
so that they go back to being errors and adds `eslint-disable`
directives above the lines that cause the violations. Note that this is
not intended to be a long-term solution, and in the future, we should
dedicate time to either fixing the violations or explaining why ignoring
them is necessary.
@mcmire
mcmireforce-pushed the remove-eslint-warnings branch from 3c84ac6 to 4476162CompareJune 6, 2024 20:02
@mcmiremcmire changed the title Remove ESLint warningsRestore ESLint warnings as errors (ignoring them for now)Jun 6, 2024
MajorLift

This comment was marked as duplicate.

@MajorLiftMajorLift left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Hopefully the presence of these comments nudge people towards providing an explanation when disabling lint rules in general.

In future PRs, enabling the 'prefer' rules should remove the need for some style guide entries, and no-unnecessary-type-assertion should also be helpful for minimizing redundant or silent-failure assertions. I can create a PR for those if you don't already have them planned @mcmire

@mcmire

Copy link
Copy Markdown
CollaboratorAuthor

@MajorLift I don't have plans for the changes you mention, but they sound good, so feel free to create tickets/PRs!

@rekmarksrekmarks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My terminal thanks you for this change. 🙇

Sign up for freeto 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.

Add eslint-disable lines for ESLint warnings

3 participants

@mcmire@rekmarks@MajorLift