-
Notifications
You must be signed in to change notification settings - Fork 133
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
Run eslint on the scripts directory #1529
Conversation
9a0f0c8
to
9d337c3
Compare
I noticed that the `scripts` directory, which begins to have a lot of files as we automate more tasks (like #1524), wasn't linted. That directory contains code in two languages BASH and JS (Node.js), with a move to prefer JS files (as it's a JS/TS project). So I chose to add an `.eslintrc.js` file (which is a deprecated format but I couldn't make the new one work) and lint script on that directory. I subsequently fixed some linting errors. For now, that script isn't run by our GitHub actions, I will see whether we can do an intelligent kind of runner which detects if scripts have been updated.
9d337c3
to
cb034f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having multiple workflow for linting different parts of the project separately is a bit overkill for the current size of the project.
If the goal is to speed up the lint process, I'm not sure it's relevant because linting all files already takes less than 1 minutes on the CI, and a more effective way would be to switch to a more recent lint engine like biome or oxlint.
I would rather have only one script npm run lint
that does eslint .
to lint all the files at once.
I got fooled more than once by running npm run lint
but it didn't catch lint errors because I had to manually run npm run lint:tests
or npm run lint:demo
.
That would also help contributers to familiarise with the project because running only one command npm run lint
is a common usage.
.github/workflows/tests_check.yml
Outdated
- tests/** | ||
|
||
jobs: | ||
scripts_lint: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's named "scripts_lint" because it's a script that lint or because it lint the "script" folder ?
here it's the test folder so it shoud not be named this way :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I forgot updating the job name here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed that typo
Just to divide multiple subjects here:
We could rename our npm scripts. Though in reality I run less and less eslint locally because I grew tired of its slowness, even when just checking |
258b81d
to
15bcb3d
Compare
I noticed that the
scripts
directory, which begins to have a lot of files as we automate more tasks (like #1524), wasn't linted.That directory contains code in two languages BASH and JS (Node.js), with a move to prefer JS files (as it's a JS/TS project).
So I chose to add an
.eslintrc.js
file (which is a deprecated format but I couldn't make the new one work) and lint script on that directory.I subsequently fixed some linting errors.
I also added GH actions running lint and type checking when various directories are updated