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

fix: Enforce lint at CI and fix existing lint warnings #181

Closed
wants to merge 2 commits into from

Conversation

@sstur
Copy link
Collaborator

sstur commented Mar 22, 2020

What does this PR accomplish?

Fixes #180

Enforces existing lint style. Makes minimum code changes to get lint passing.

npm run test now runs lint also. This ensures lint will be run during CI build.

Note: this does not change the eslintConfig key in package.json to use gts. I think that should be a different PR since it will likely surface new code style issues (but I have not yet confirmed that it works).

Did you add any dependencies?

No.

How did you test the change?

npm run test

Copy link
Collaborator

epicfaace left a comment

Looks good, please see my comment at #180 (comment) though

@@ -31,11 +31,13 @@
"scripts": {
"start": "react-scripts start",
"build": "react-scripts build",
"test": "react-scripts test",
"test": "npm run lint && react-scripts test",

This comment has been minimized.

Copy link
@epicfaace

epicfaace Mar 22, 2020

Collaborator

Rather than running this, it might just be good to set the CI=true environment variable when running react-scripts test -- react-scripts will then automatically treat eslint warnings as errors (see the change I tried in #168). This will marginally speed up the CI build.

This comment has been minimized.

Copy link
@sstur

sstur Mar 23, 2020

Author Collaborator

OK, that makes sense.

This comment has been minimized.

Copy link
@hspinks

hspinks Mar 23, 2020

Collaborator

@epicfaace to your comment on #168, did you ever figure out where the different eslint configs were coming from? definitely doesn't seem right for there to be 2 configs that could confuse devs when run at different times in the build process...

This comment has been minimized.

Copy link
@epicfaace

epicfaace Mar 23, 2020

Collaborator

Yes, we discussed it further here -- #180. We probably shouldn't merge this PR as-is because as you said, it would cause two configs to be there.

"eject": "react-scripts eject",
"check": "gts check",
"cleanbuild": "gts clean",
"compile": "tsc -p .",
"typecheck": "tsc --noEmit -p .",

This comment has been minimized.

Copy link
@epicfaace

epicfaace Mar 22, 2020

Collaborator

Do we need this given that gts check (I believe) already does a typecheck?

This comment has been minimized.

Copy link
@sstur

sstur Mar 23, 2020

Author Collaborator

Right, actually from what I can tell it's pretest that runs compile (the line above) which catches the type errors during CI (although I'm not sure if gts check also runs this). I added this just to expose a way to run the typecheck at the cli, but not sure if it's necessary.

@sstur

This comment has been minimized.

Copy link
Collaborator Author

sstur commented Mar 23, 2020

Looks good, please see my comment at #180 (comment) though

OK, continuing this conversation over in that issue.

Copy link
Collaborator

hspinks left a comment

pending the question about separate eslint configs, this lgtm

@hspinks hspinks added this to In progress in Engineering via automation Mar 24, 2020
@sstur

This comment has been minimized.

Copy link
Collaborator Author

sstur commented Mar 25, 2020

OK, it seems the decision was made to ditch react-scripts eslint config in favor of gts v2, the plan that was laid out over at #180. So I'll close this PR for the time being.

@sstur sstur closed this Mar 25, 2020
Engineering automation moved this from In progress to Done Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Engineering
  
Done
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.