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

Run eslint on the scripts directory #1529

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Conversation

peaBerberian
Copy link
Collaborator

@peaBerberian peaBerberian commented Sep 4, 2024

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

@peaBerberian peaBerberian force-pushed the misc/scripts-eslint branch 2 times, most recently from 9a0f0c8 to 9d337c3 Compare September 4, 2024 09:41
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.
Copy link
Collaborator

@Florent-Bouisset Florent-Bouisset left a 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.

- tests/**

jobs:
scripts_lint:
Copy link
Collaborator

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 :)

Copy link
Collaborator Author

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed that typo

@peaBerberian
Copy link
Collaborator Author

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.

Just to divide multiple subjects here:

  1. there's multiple .eslintrc.js files because the src directory does not obey the same rules than the demo, the tests or the scripts directory (e.g. scripts run in a Node.js environment, src in a browser, demo has react etc.)

  2. The workflows are separate not really for speed, but to avoid doing unnecessary checks for nothing and having too many actions running per PR: it already seems to be superior to what "github view" shows by default, forcing me to scroll in their interface.

    We could group all lints task under the same workflow, yet :
    - having a false negative due to an issue randomly seen in the scripts directory would be bothersome
    - We don't even need to run that script in that case, and I'm not comfortable with the idea of taking computing resources that are not even needed when that's easy to fix
    - Now that I'm thinking of speed, demo linting for example might take time where in 99% of cases it's not even needed.

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.

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 src.
But If external contributors wants to have a perfect contribution before PR-time (like I would guess most of them), sure, we could have a global npm run lint scripts and sub-scripts (what would the src script be called though, if there was one?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants