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

ci: added linting ci in the repo #296

Closed
wants to merge 2 commits into from
Closed

ci: added linting ci in the repo #296

wants to merge 2 commits into from

Conversation

Gmin2
Copy link
Contributor

@Gmin2 Gmin2 commented Feb 8, 2024

Fixes #289

follow up to #290

tested locally here

image

@Gmin2
Copy link
Contributor Author

Gmin2 commented Feb 12, 2024

I think we can merge this @Relequestual

Copy link
Member

@Relequestual Relequestual left a comment

Choose a reason for hiding this comment

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

Thanks for this.
I'm not sure about running lint-fix.
What will that do in the context of a GitHub action?

Have you confirmed that it will show any errors? =]

@Gmin2
Copy link
Contributor Author

Gmin2 commented Feb 12, 2024

The lint-fix scrip runs a linter with an automatic fix option enabled. In the context of a GitHub Action, running npm run lint-fix as mentioned in the script will execute a command that checks your code for style or syntax errors and attempts to automatically fix them where possible.

image

Regarding errors, if the code has any style or syntax errors that can't be automatically fixed, the lint-fix command may not resolve them entirely. However, it can still be valuable in catching and fixing many common issues automatically.

cc @Relequestual

@Relequestual
Copy link
Member

Relequestual commented Feb 12, 2024

In the context of a GitHub Action, running npm run lint-fix as mentioned in the script will execute a command that checks your code for style or syntax errors and attempts to automatically fix them where possible.

It may fix the code it has, but it won't commit it back to the repo.
When the PR is from a fork, it won't have permissions to commit in that fork.
When the PR is not from a fork, but a local branch, you could commit, but this currently does not.

I don't think having autofix here makes sense. Open to other opinions.

Have you confirmed that it will show any errors?

Regarding errors, if the code has any style or syntax errors that can't be automatically fixed, the lint-fix command may not resolve them entirely. However, it can still be valuable in catching and fixing many common issues automatically.

I meant, have you confirmed that when there are linting errors, they are shown in the action output and make the check fail?

@Gmin2
Copy link
Contributor Author

Gmin2 commented Feb 12, 2024

It may fix the code it has, but it won't commit it back to the repo. When the PR is from a fork, it won't have permissions to commit in that fork. When the PR is not from a fork, but a local branch, you could commit, but this currently does not.

Thanks for pointing that out. You're right; the lint-fix script may automatically fix code issues but won't commit them back to the repository.

I don't think having autofix here makes sense. Open to other opinions.

I agree that having autofix in this context might not be the best approach.

I meant, have you confirmed that when there are linting errors, they are shown in the action output and make the check fail?

If there are linting issues , the lint scripts will cause the workflow to fail since it will return a non-zero exit code for errors

@benjagm
Copy link
Collaborator

benjagm commented Feb 24, 2024

@Min2who I think this functionality is going to be covered by this PR #335 right @aialok?

@aialok
Copy link
Member

aialok commented Feb 24, 2024

@Min2who I think this functionality is going to be covered by this PR #335 right @aialok?

@benjagm
No, the functionality in #335 and the functionality in this issue are completely different. Both are performing the same task, but they serve different purposes. This functionality adds the same checks, but in the Continuous Integration Pipeline, specifically in our GitHub Actions workflow, whereas the husky one will be a local check only. The husky one will check these conditions only when we try to commit locally.

There are cases where contributors bypass husky checks somehow. This is why we need to set up a CI pipeline in GitHub Actions for each PR to review whether it has passed the checks or not. This is crucial for the overall stability and safety of the project.

I have already discussed in this issue: #340 (comment)

This issue and #340 are addressing the same things, but #340 also includes checking for formatting and type checking.

@Gmin2 Gmin2 closed this Feb 28, 2024
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.

Improve the CI/CD pipeline to add linting in the repository
4 participants