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.

Sign up for GitHub

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

Jump to bottom

Enable pre-commit.ci for style checking #197

Merged
merged 5 commits into from
Jul 11, 2024
Merged

Enable pre-commit.ci for style checking #197

merged 5 commits into from
Jul 11, 2024

Conversation

flferretti
Copy link
Collaborator

@flferretti flferretti commented Jul 5, 2024 •
edited by github-actions bot
Loading

This PR removes the current style checking from GitHub Actions and moves it to pre-commit.ci, this will allow for additional checking.

Closes #195

📚 Documentation preview 📚: https://jaxsim--197.org.readthedocs.build//197/

@flferretti flferretti self-assigned this Jul 5, 2024
@flferretti flferretti force-pushed the flferretti-patch-2 branch from 08ffcdb to e467fe7 Compare July 5, 2024 11:31
@flferretti flferretti marked this pull request as ready for review July 5, 2024 11:31
@flferretti flferretti requested a review from diegoferigo as a code owner July 5, 2024 11:31
@flferretti flferretti mentioned this pull request Jul 5, 2024
Merged
@diegoferigo
Copy link
Member

Before reviewing this PR, a curiosity. Is there any way to perform this check without relying on external services? And I mean, can we do it using a regular GitHub workflow?

@flferretti
Copy link
Collaborator Author

Before reviewing this PR, a curiosity. Is there any way to perform this check without relying on external services? And I mean, can we do it using a regular GitHub workflow?

Yes, there is the lite version of the CI, you can find more details at https://pre-commit.ci/lite.html

@flferretti
Copy link
Collaborator Author

Btw I temporarily disabled the pre-commit.ci integration from the repo as it got super annoying with the automatic commit fixes

@diegoferigo
Copy link
Member

If there is an option providing all the features we need that doesn't use any external service, I'd rather go towards that way. Many years ago, we were interfacing with similar external services (like CirleCI, TravisCI, etc), and they are definitely more prone to get broken, be down, or have some sort of problems.

Would it require a big effort enabling the lite version as GitHub Workflow?

@flferretti
Copy link
Collaborator Author

Would it require a big effort enabling the lite version as GitHub Workflow?

Totally not, I'll add that

@flferretti flferretti force-pushed the flferretti-patch-2 branch 3 times, most recently from 83bd105 to 0fe97f8 Compare July 5, 2024 14:55
@flferretti
Copy link
Collaborator Author

After discussing with @diegoferigo, we decided to use the pre-commit.ci instead of the GitHub Actions integration as it is faster and it is actively maintained.

I pushed a dummy commit to verify that the CI actually fails when the formatting is wrong. This can be fixed in three ways:

  1. use pre-commit hook (suggested)
  2. trigger the bot commenting with pre-commit.ci autofix
  3. adding a pre-commit.ci autofix label (to avoid)

@flferretti
Copy link
Collaborator Author

pre-commit.ci autofix

@flferretti flferretti force-pushed the flferretti-patch-2 branch 2 times, most recently from edbbd96 to 5380f40 Compare July 9, 2024 13:57
@flferretti
Copy link
Collaborator Author

I dropped the last commit, for some reason the bot doesn't work. It may be due to some issue upstream, but I'll verify that. Ready for review @diegoferigo, thanks a lot!

diegoferigo
diegoferigo approved these changes Jul 10, 2024
View reviewed changes
.github/workflows/ci_cd.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@diegoferigo
Copy link
Member

Can you please also cherry pick the updates of #200 here?

@flferretti flferretti mentioned this pull request Jul 10, 2024
Closed
@flferretti
Update Python version
c25a577
@flferretti
[pre-commit.ci] pre-commit autoupdate
b79406c 
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.5.0 → v4.6.0](pre-commit/pre-commit-hooks@v4.5.0...v4.6.0)
- [github.com/psf/black-pre-commit-mirror: 24.2.0 → 24.4.2](psf/black-pre-commit-mirror@24.2.0...24.4.2)
- [github.com/astral-sh/ruff-pre-commit: v0.3.2 → v0.5.1](astral-sh/ruff-pre-commit@v0.3.2...v0.5.1)
@flferretti flferretti force-pushed the flferretti-patch-2 branch from c6b9cea to b79406c Compare July 11, 2024 08:07
@flferretti
Copy link
Collaborator Author

Can you please also cherry pick the updates of #200 here?

Done in b79406c.

Would you like to add anything? Otherwise I'll merge this

diegoferigo
diegoferigo approved these changes Jul 11, 2024
View reviewed changes
Copy link
Member

@diegoferigo diegoferigo left a comment

Choose a reason for hiding this comment

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

Nope, good to go, thanks!

@flferretti flferretti merged commit 2056e70 into main Jul 11, 2024
24 checks passed
@flferretti flferretti deleted the flferretti-patch-2 branch July 11, 2024 10:00
@flferretti
Copy link
Collaborator Author

FYI @Giulero

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diegoferigo diegoferigo diegoferigo approved these changes

Assignees

@flferretti flferretti

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider using pre-commit.ci for style checking in CI
2 participants
@flferretti @diegoferigo