Added Linting as a part of CI #393
Conversation
|
@hoch please review the PR, at your convenience. Thanks :) |
|
hi @hoch. Any update on this? |
|
Hi @hoch, Any update on this PR? |
|
Apologies. I am currently swamped by urgent tasks at work. I'll ask around if other team members are able to review this change. |
There was a problem hiding this comment.
Hi @tarunsinghofficial, thank you for raising and working on this issue. I'll take on the review since Hongchan has been overloaded recently.
The overall approach looks good to me. I left some comments, please take a look and let me know what you think.
| - name: Run ESLint | ||
| run: npm run lint:eslint | ||
|
|
||
| - name: Lint changed files |
There was a problem hiding this comment.
It seems like the above "Run Prettier" and "Run ESLint" steps will run the linters on all files, and then this step will run the linters again just on the changed files. That seems redundant to me. Is it possible to either run on all files, or just run on the changed files? Or am I not correctly understanding what's happening?
There was a problem hiding this comment.
Yes, we can just run linting on the changed files, so that there is no redundancy.
There was a problem hiding this comment.
Can we remove the "Run Prettier" step above too?
| @@ -0,0 +1,4 @@ | |||
| # Package Managers | |||
There was a problem hiding this comment.
I don't think it's a problem to exclude these explicitly, but it seems like the commands added in package.json as well as the grep in the "Lint changed files" step will already avoid passing these files to the linters.
There was a problem hiding this comment.
Removed unnecessary exclusions since they're already excluded by other parts of the config.
There was a problem hiding this comment.
Can we remove this entire file now?
| @@ -12,6 +12,8 @@ | |||
| "start:eleventy": "eleventy --serve", | |||
| "start:postcss": "postcss src/styles/*.css --dir _site --watch", | |||
| "format": "npx eslint --fix _site/audio-worklet/**/*.js && npx prettier --write --loglevel silent _site/audio-worklet/**/*.html", | |||
| "lint:prettier": "prettier --check src/**/*.js src/**/*.html", | |||
There was a problem hiding this comment.
If we remove the "Lint changed files" step we may want to add the other relevant filetypes here.
There was a problem hiding this comment.
I decided to keep the "Lint changed files" step, which runs linters only on the changed files. As a result, there's no need to modify the package.json file to include additional file types for linting, since the dynamic filtering for relevant files is handled in the "Lint changed files" step itself.
There was a problem hiding this comment.
Can we remove the "lint:prettier" and "lint:eslint" lines now too?
| - name: Run ESLint | ||
| run: npm run lint:eslint | ||
|
|
||
| - name: Lint changed files |
There was a problem hiding this comment.
Can we remove the "Run Prettier" step above too?
| @@ -0,0 +1,4 @@ | |||
| # Package Managers | |||
There was a problem hiding this comment.
Can we remove this entire file now?
| @@ -12,6 +12,8 @@ | |||
| "start:eleventy": "eleventy --serve", | |||
| "start:postcss": "postcss src/styles/*.css --dir _site --watch", | |||
| "format": "npx eslint --fix _site/audio-worklet/**/*.js && npx prettier --write --loglevel silent _site/audio-worklet/**/*.html", | |||
| "lint:prettier": "prettier --check src/**/*.js src/**/*.html", | |||
There was a problem hiding this comment.
Can we remove the "lint:prettier" and "lint:eslint" lines now too?
| run: npm run lint:prettier | ||
|
|
||
| - name: Lint changed files | ||
| if: github.event_name == 'pull_request' |
There was a problem hiding this comment.
Since this step does the actual linting, and it only runs on 'pull_request', we should probably do one of the following:
- Either remove the
pushline from theonsection at the top of the file - Or, allow this linting to run on both push and pull request
What do you think?
Overview
This PR introduces and fixes #392 for automated linting to our Continuous Integration (CI) pipeline. It aims to ensure consistent code style and catch potential errors early in the development process.
Changes
.github/workflows/ci.yml.prettierignoreto ignore some package managersImplementation Details
The new CI workflow does the following:
Notes
Please review and let me know if any changes or additions are needed.