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

Added Linting as a part of CI #393

Merged

Conversation

tarunsinghofficial
Copy link
Contributor

@tarunsinghofficial tarunsinghofficial commented Aug 23, 2024

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

  • Added a new GitHub Actions workflow file: .github/workflows/ci.yml
  • Configured the workflow to run on push to main/master branches and on pull requests
  • Added a .prettierignore to ignore some package managers
  • Implemented Prettier and ESLint checks for all files
  • Added specific checks for changed files in pull requests

Implementation Details

The new CI workflow does the following:

  1. Check out the code
  2. Sets up Node.js
  3. Installs dependencies
  4. Runs Prettier on all files
  5. Runs ESLint on all files
  6. For pull requests, run Prettier and ESLint on changed files only

Notes

  • Caching of npm artifacts was considered but not implemented. We can add this later if build times become an issue.
  • The workflow is configured to use Node.js version 18. We may need to update this in the future.

Please review and let me know if any changes or additions are needed.

@tarunsinghofficial
Copy link
Contributor Author

tarunsinghofficial commented Aug 26, 2024

@hoch please review the PR, at your convenience. Thanks :)

@tarunsinghofficial
Copy link
Contributor Author

hi @hoch. Any update on this?

@tarunsinghofficial
Copy link
Contributor Author

Hi @hoch, Any update on this PR?

@hoch
Copy link
Member

hoch commented Nov 29, 2024

Apologies. I am currently swamped by urgent tasks at work. I'll ask around if other team members are able to review this change.

@mjwilson-google mjwilson-google self-requested a review January 2, 2025 18:48
Copy link
Collaborator

@mjwilson-google mjwilson-google left a comment

Choose a reason for hiding this comment

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

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.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.prettierignore Outdated Show resolved Hide resolved
.prettierrc.json Outdated Show resolved Hide resolved
.prettierrc.json Outdated Show resolved Hide resolved
.prettierrc.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@mjwilson-google mjwilson-google left a comment

Choose a reason for hiding this comment

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

Thank you, looking even better! Just a few more comments.

.github/workflows/ci.yml Show resolved Hide resolved
.prettierignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
…lint:eslint from package.json, and remove 'push' trigger from lint workflow
@tarunsinghofficial
Copy link
Contributor Author

Thanks for the feedback @mjwilson-google. Done with all the changes and is ready to 🚀 :)

Copy link
Collaborator

@mjwilson-google mjwilson-google left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you for your work on this.

@mjwilson-google mjwilson-google merged commit 16fe2be into GoogleChromeLabs:main Jan 6, 2025
3 of 4 checks passed
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.

Add Linting as a part of CI pipeline
3 participants