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

Add ESLint and prettier #173

Closed
wants to merge 7 commits into from
Closed

Add ESLint and prettier #173

wants to merge 7 commits into from

Conversation

@rachelrf
Copy link
Collaborator

rachelrf commented Mar 24, 2020

run yarn lint to lint and yarn lint-fix to automatically fix lint issues

Copy link
Contributor

szhu left a comment

+1 to this change, I was thinking of adding something similar.

But, just because you've run it on all the files doesn't mean that future contributors will run it on code they add, since things will just get messy again over time. (I know that there are editor integrations that devs can set up, but it's hard to tell everyone to use them.)

I think we should do the following before merging this PR:

  • (very critical) Add a git pre-commit hook that does checks. I've had a good experience using lint-staged -- it's very seamless and sets everything up as long as the user runs npm install or yarn install. We can (and should) set it up to run prettier for you instead of potentially just complaining that your file isn't conforming.
    • Let me know here or on Slack if you have questions about setting this up -- I can also push a commit adding this into this PR if that's more convenient for you.
  • (probably also a good idea) Add a ci check to fail the build when linting is failing.
rachelrf added 3 commits Mar 24, 2020
"extends": "react-app"
"lint-staged": {
"src/**/*.{css,scss,js}": [
"eslint --fix src"

This comment has been minimized.

Copy link
@szhu

szhu Mar 24, 2020

Contributor

Thanks! Can you also include the prettier --write command?

This comment has been minimized.

Copy link
@rachelrf

rachelrf Mar 24, 2020

Author Collaborator

It's not super obvious but I've configured eslint with a plugin[1] which will run prettier before linting, and running with --fix will run it in --write mode.

[1] https://github.com/covid-projections/covid-projections/pull/173/files#diff-e4403a877d80de653400d88d85e4801aR7

This comment has been minimized.

Copy link
@szhu

szhu Mar 24, 2020

Contributor

oh cool! thanks for adding and pointing that out.

@rachelrf

This comment has been minimized.

Copy link
Collaborator Author

rachelrf commented Mar 24, 2020

Good call -- added the pre-commit hook.

I don't have access to the CI server/Netlify, so it's hard to set up a lint check (I can't see the logs when checks are failing). Do you know who could give me access?

If I can get CI access and verify the checks there, I think this will be good to go.

@szhu

This comment has been minimized.

Copy link
Contributor

szhu commented Mar 24, 2020

Honestly, no idea about the ci either. Maybe ask #webapp-eng? The CI stuff can probably be added in another PR actually.

I'm fairly new to this project, which is why I didn't hit approve (not sure if there's a sign-off process), but it looks good to me now!

@szhu szhu dismissed their stale review Mar 24, 2020

Addressed

@zjrosen1

This comment has been minimized.

Copy link
Contributor

zjrosen1 commented Mar 24, 2020

rebased develop and merged on another branch

@zjrosen1 zjrosen1 closed this Mar 24, 2020
This was referenced Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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