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

Makes linting run before code is committed #4132

Merged
merged 10 commits into from
May 19, 2022

Conversation

madsnedergaard
Copy link
Member

@madsnedergaard madsnedergaard commented May 16, 2022

Description

We want to improve linting and make the codebase more standardized.

This PR makes linting and formatting happen automatically before code is committed.

Steps:

@github-actions github-actions bot added dependencies Pull requests that update a dependency file frontend 🎨 labels May 16, 2022
This was referenced May 16, 2022
@VIKTORVAV99
Copy link
Member

Consider running prettier on the /.circleci, /.github and /config directories/folders as well.

Probably won't do much on the first two but ruining it on the last would be nice so the config files all have the same formatting throughout. (mostly affecting zones.json and exchanges.json)

@madsnedergaard
Copy link
Member Author

Oh yes, that's a great point! Since those are outside of web and are often edited through the browser, maybe running it in a GH action or CI like you suggested would be ideal? :)

@VIKTORVAV99
Copy link
Member

Sounds like we have a plan then!
I'll create a PR after the others are merged.

@madsnedergaard madsnedergaard force-pushed the mn/linting/make-prettier-run-on-commit branch from 31c0206 to 520537f Compare May 17, 2022 11:03
@madsnedergaard
Copy link
Member Author

Sounds like we have a plan then! I'll create a PR after the others are merged.

Awesome!
We are actually already running a "lint JSON" step in CircleCI:

sudo npm install -g jsonlint-mod
jsonlint -q config/*.json web/public/locales/*.json

@VIKTORVAV99
Copy link
Member

Sounds like we have a plan then! I'll create a PR after the others are merged.

Awesome! We are actually already running a "lint JSON" step in CircleCI:

sudo npm install -g jsonlint-mod
jsonlint -q config/*.json web/public/locales/*.json

This would differ somewhat though, prettier formatting can fail even if linting passes.
An example would be:

"capacity": [
      -533,
      533
    ]

and

"capacity": [-533, 533]

both will pass linting but only the second one will pass prettier.

Now just adding prettier to check if the formatting is right is as simple as it gets but if we want prettier for format it too it gets a little more complicated as it will create a new commit but still very doable (I am personally running it this way in a private repo with a github actions workflow).
I can't say I know off hand how to do it in CircleCI but it can't be that hard to figure out.

@madsnedergaard
Copy link
Member Author

Now just adding prettier to check if the formatting is right is as simple as it gets but if we want prettier for format it too it gets a little more complicated as it will create a new commit but still very doable (I am personally running it this way in a private repo with a github actions workflow). I can't say I know off hand how to do it in CircleCI but it can't be that hard to figure out.

Hmm, the committing changes part is actually a problem with forks (see discussion here: #4131) 🤔
Not sure how to best approach it, but maybe just checking (and failing on changes) is an okay first step? We could reuse/change the lint_json step in CircleCI to do this.

@madsnedergaard madsnedergaard marked this pull request as ready for review May 17, 2022 14:46
@VIKTORVAV99
Copy link
Member

I left a comment on the other PR but yeah just doing a check seems like the only option for now and it should be fine for most scenarios.

While reusing existing jobs are a option I think we want to do as much in parallel as we can to reduce CI run durations.
So it might make sense to create a separate job for it, would be easier to tell if it was linting or formatting that failed too.

.circleci/config.yml Outdated Show resolved Hide resolved
Comment on lines -84 to +86
"nodemon": "^1.17.1",
"nodemon": "^2.0.16",
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixed the annoying problem when running yarn install and seeing a bunch of errors from fsevents/node-pre-gyp :)

Copy link
Member

Choose a reason for hiding this comment

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

It also fixes a bunch of security vulnerabilities. 👀

I was actually planning on submitting a PR for it after the current PRs that target the lock file was merged.

Copy link
Contributor

@Kongkille Kongkille left a comment

Choose a reason for hiding this comment

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

Excellent 👌

Edit: I'm just going to go ahead and fix the merge conflicts

@Kongkille Kongkille merged commit ac98dc7 into master May 19, 2022
@Kongkille Kongkille deleted the mn/linting/make-prettier-run-on-commit branch May 19, 2022 13:57
@madsnedergaard
Copy link
Member Author

Thanks for fixing and merging ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file frontend 🎨 infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants