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

Issue #340 - Feature: CI(Github Actions) - Performs linting, formatting, and type-checking #432

Merged
merged 8 commits into from
Mar 5, 2024

Conversation

ayushtiwari110
Copy link
Contributor

@ayushtiwari110 ayushtiwari110 commented Mar 3, 2024

What kind of change does this PR introduce?

  • Implements a new CI workflow for code quality checks such as linting, formatting, type-checking, as well as build checks.

Issue Number:

Summary
This PR sets up a CI workflow triggered on pull requests. The workflow includes the following jobs:

Linting and Type-Checking: Ensures code style consistency and catches potential type errors.
Build: Verifies that the project can be successfully built.
The workflow is designed to improve code quality and maintainability by catching potential issues early in the development process.

Does this PR introduce a breaking change?
No, this PR does not introduces breaking changes...

Copy link
Collaborator

@aialok aialok left a comment

Choose a reason for hiding this comment

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

See Comments.

.github/workflows/code-checker.yml Outdated Show resolved Hide resolved
.github/workflows/code-checker.yml Outdated Show resolved Hide resolved
.github/workflows/code-checker.yml Outdated Show resolved Hide resolved
@aialok
Copy link
Collaborator

aialok commented Mar 4, 2024

Thank You @ayushtiwari110, You have done amazing job : )

  • Can you please resolve the suggested changes.
  • One more things, Can we cache our dependencies so we can speed up our workflows.

Here are some reference you can take help from :

Ping me. If you need any help :)

cc : @benjagm thoughts : )

@ayushtiwari110
Copy link
Contributor Author

ayushtiwari110 commented Mar 4, 2024

Hey @aialok , firstly thanks for reviewing the PR.
I have added the requested changes and have cached the dependencies. You can check and let me know of any further changes required. Thanks!
@benjagm if there are any suggestions from your side, I would be glad to hear them!

@benjagm
Copy link
Collaborator

benjagm commented Mar 4, 2024

Thanks everyone for the great work here. As soon as @aialok approves I'd do my review.

@benjagm benjagm requested a review from aialok March 4, 2024 13:31
Copy link
Collaborator

@aialok aialok left a comment

Choose a reason for hiding this comment

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

See Comments. Other than this everything look good to me.
Can you please change the workflow file name to pull-request.yml. It will make more sense as we are using same naming convention in other workflow too.
Thank you @ayushtiwari110 : )

@aialok
Copy link
Collaborator

aialok commented Mar 4, 2024

my bad....I forget to click on add comment : (

@ayushtiwari110
Copy link
Contributor Author

@aialok I have changed the workflow file name as per conventions. Kindly check.
Do let me know in case of any update. Thanks!🚀

@ayushtiwari110
Copy link
Contributor Author

I guess the cache process for installation of dependencies must be same for npm and yarn? Could you please clarify once
for Next build job,I'll update the code as per their documentation.

@ayushtiwari110
Copy link
Contributor Author

ayushtiwari110 commented Mar 4, 2024

I guess the cache process for installation of dependencies must be same for npm and yarn? Could you please clarify once for Next build job,I'll update the code as per their documentation.

Ok well I got it, The modules path will be different for yarn. I'll update the requested changes...

@aialok
Copy link
Collaborator

aialok commented Mar 4, 2024

I guess the cache process for installation of dependencies must be same for npm and yarn? Could you please clarify once for Next build job,I'll update the code as per their documentation.

Ok well I got it, The modules path will be different for yarn. I'll update the requested changes...

https://github.com/actions/cache/blob/main/examples.md#node---yarn

you can check out this : )

@ayushtiwari110
Copy link
Contributor Author

for next build, shall i cache both the build files and the node modules?

@ayushtiwari110
Copy link
Contributor Author

@aialok I have added the cache for next build, and have done the requested changes. Kindly check the same. Thanks!

Copy link
Collaborator

@aialok aialok left a comment

Choose a reason for hiding this comment

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

@ayushtiwari110 see comments. Other than this everything LGTM : )

Thank You so much @ayushtiwari110 . It was really fun working with you.

cc : @benjagm please review : )

@ayushtiwari110
Copy link
Contributor Author

@aialok The final requested changes have been implemented.
Thank you for your feedback throughout this pull request. I've also enjoyed collaborating with you on this PR! 🚀

@benjagm As alok mentioned, the changes are ready for your review. I appreciate your feedback on this :-)

Copy link
Collaborator

@benjagm benjagm left a comment

Choose a reason for hiding this comment

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

Amazing job with this one! Let's merge it!

@benjagm
Copy link
Collaborator

benjagm commented Mar 5, 2024

Is this resolving #340?

@ayushtiwari110
Copy link
Contributor Author

@benjagm yes this PR is meant for #340

@ayushtiwari110 ayushtiwari110 marked this pull request as ready for review March 5, 2024 12:01
@benjagm benjagm merged commit c80509b into json-schema-org:main Mar 5, 2024
3 checks passed
@DarhkVoyd
Copy link
Member

@aialok @benjagm While reviewing another GitHub workflow PR, I have noticed that this workflow would fail each time a new dependency is added to the project. I believe it to be originating from this line.

yarn install --frozen-lockfile

@aialok
Copy link
Collaborator

aialok commented Mar 6, 2024

@aialok @benjagm While reviewing another GitHub workflow PR, I have noticed that this workflow would fail each time a new dependency is added to the project. I believe it to be originating from this line.

yarn install --frozen-lockfile

Thank @DarhkVoyd for your review. But this will not happen because when you run yarn add without the --frozen-lock flag, Yarn will update the yarn.lock file if it needs to install new dependencies or update existing ones to satisfy the package requirements specified in package.json. This can lead to the yarn.lock file being modified, potentially causing discrepancies between development environments.

By using the --frozen-lock flag, you are essentially telling Yarn not to update the yarn.lock file during the installation process. This ensures that the exact versions of dependencies specified in the yarn.lock file are used, maintaining consistency across different environments and preventing unintended changes to dependencies.

As we are installing dependencies locally and then pushing them to GitHub, we already have an updated version of the yarn.lock file. Therefore, using the --frozen-lock flag will only install the dependencies specified in the yarn.lock file. This will not affect our Continuous Integration (CI) process.

If you have any question feel free to ping me : )

@DarhkVoyd
Copy link
Member

Got it, my bad.

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.

Feature: CI(Github Actions) - Performs linting, formatting, and type-checking
4 participants