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: add lint and format check #208

Conversation

kazizi55
Copy link
Contributor

@kazizi55 kazizi55 commented Oct 9, 2023

Since the number of implemented linter rules is increasing (#205 #202) and will be increased very much, I added lint (and format) check to CI.

This will reduce the review time and effort 😄

@netlify
Copy link

netlify bot commented Oct 9, 2023

Deploy Preview for valibot canceled.

Name Link
🔨 Latest commit 66be71d
🔍 Latest deploy log https://app.netlify.com/sites/valibot/deploys/652cb35886b60d00082463cf

@kazizi55 kazizi55 marked this pull request as ready for review October 9, 2023 11:29
@fabian-hiller
Copy link
Owner

Thank you! This is great! I think it makes sense to do the format and lint checks before the tests. What do you think about that? Or is it better to run each check independently in its own pipeline? Do you know how other projects do this?

@fabian-hiller fabian-hiller self-assigned this Oct 9, 2023
@fabian-hiller fabian-hiller added github GitHub related changes enhancement New feature or request labels Oct 9, 2023
@kazizi55
Copy link
Contributor Author

@fabian-hiller
Thank you too 😄

I think it makes sense to do the format and lint checks before the tests. What do you think about that?

Yes, I think so too, because the format and lint checks don’t usually take so much time, therefore there is a slight time difference between running all in series and running each check in parallel.

Do you know how other projects do this?

Here are some examples:

@fabian-hiller
Copy link
Owner

Right now I think it's best to run everything in parallel and list each check individually on a PR so you know exactly what doesn't fit. What do you think?

@fabian-hiller
Copy link
Owner

Thank you for your work. I think something is not configured properly yet. At least not all the checks are running yet.

@fabian-hiller
Copy link
Owner

Did you configure that the job install must run first before the others are allowed to start?

@kazizi55
Copy link
Contributor Author

Thank you for your quick response!
I'll check the detail 🙏

@kazizi55
Copy link
Contributor Author

Fixed! 😄
b7ec31d

Right now I think it's best to run everything in parallel and list each check individually on a PR so you know exactly what doesn't fit. What do you think?

I think both are acceptable now, but in the long term, running everything in parallel will be better, because of the expectation of the increase of CI check, which causes delay.

@fabian-hiller
Copy link
Owner

Thank you for your contribution! I will review your changes in the next 24 hours.

Copy link
Owner

@fabian-hiller fabian-hiller 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 so much for improving this. I find this very useful for all future PRs. The code looks good. I just want to rename a few labels. Once I have done that, I will merge the PR.

@kazizi55
Copy link
Contributor Author

Thank you for your words! 😄

I just want to rename a few labels. Once I have done that, I will merge the PR.

OK, go ahead!

@fabian-hiller fabian-hiller force-pushed the feature/add-lint-and-format-check-to-ci branch 2 times, most recently from 57a013d to 65bc232 Compare October 16, 2023 03:32
@fabian-hiller fabian-hiller force-pushed the feature/add-lint-and-format-check-to-ci branch 3 times, most recently from d6d70c8 to bc205cb Compare October 16, 2023 03:50
@fabian-hiller fabian-hiller force-pushed the feature/add-lint-and-format-check-to-ci branch from bc205cb to 66be71d Compare October 16, 2023 03:51
@fabian-hiller fabian-hiller merged commit 741715f into fabian-hiller:main Oct 16, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request github GitHub related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants