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

Set eslint's max-warnings to 0 to match pre-commit hooks #7617

Merged
merged 2 commits into from
Oct 25, 2021

Conversation

reggieriser
Copy link
Contributor

@reggieriser reggieriser commented Oct 22, 2021

Description

We had some code that had eslint warnings slip into master (see Slack thread for more context). Those warnings started to show up on subsequent PRs. Digging into how this could get through our checks, I noticed our pre-commit hooks set --max-warnings=0 on eslint which fails the hook if there are warnings (the warnings got in by accident after disabling pre-commit hooks to get code pushed up). In contrast, the eslint command we use in the build process did not have max-warnings set. This adds it.

Reviewer Notes

This PR should have some failures until we get the warnings fixed. I wanted to get a PR up and make sure that it failed on warnings, then we can merge the warning fixes in when they land and try it again and see that everything is hopefully clear. I don't want to merge this until we're clear as it may block the warning fixes from being applied. I'll make this a draft PR and label it with do not merge until everything's ready. Update: PR is now out of draft and warnings should be gone.

Let me know if you can think of anything that this would negatively impact.

Setup

Locally, you can run yarn run lint and see that the warnings return an exit code of 1 now. You should hopefully see a failure in the checks on this PR too.

…e failures now if any warnings arise just like pre-commit.
@reggieriser
Copy link
Contributor Author

Just capturing screenshots of the failures while the warnings are still in the branch.

Screen Shot 2021-10-22 at 11 11 06 AM

Screen Shot 2021-10-22 at 11 11 23 AM

Copy link
Contributor

@gidjin gidjin left a comment

Choose a reason for hiding this comment

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

LGTM, probably need to resolve the warnings before merging though

@reggieriser
Copy link
Contributor Author

LGTM, probably need to resolve the warnings before merging though

That was the plan -- see the reviewer's notes. I was just waiting for #7618 to land. Thanks for the review.

@reggieriser reggieriser marked this pull request as ready for review October 25, 2021 11:16
@robot-mymove
Copy link

Warnings
⚠️

Changes were made to package.json, but not to yarn.lock - Perhaps you need to run yarn install?

⚠️ Please add the JIRA issue key to the PR title (e.g. MB-123)

Generated by 🚫 dangerJS against 8ced292

@reggieriser reggieriser merged commit 6289e8e into master Oct 25, 2021
@reggieriser reggieriser deleted the rr-fix-eslint-maxwarnings branch October 25, 2021 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants