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

Add precommit #601

Merged
merged 2 commits into from
Mar 22, 2024
Merged

Add precommit #601

merged 2 commits into from
Mar 22, 2024

Conversation

Rotzbua
Copy link
Contributor

@Rotzbua Rotzbua commented Nov 3, 2023

Possible configuration for pre-commit as suggested in #600 .

Second commit just formats the code and may should be dropped before merge and done by maintainer.

@florianfesti
Copy link
Owner

I am not quite sure what "pre commit" means here. This looks like a GitHub hook which sounds like pre push to me. Otoh why would one need a local installation for that?

@Rotzbua
Copy link
Contributor Author

Rotzbua commented Nov 22, 2023

Git itself supports various hooks https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks

The idea is to test and format code before the commit is created. This reduces the probability that a commit would not pass CI or break the program.

I created a simpler PR which does not format the complete code. So it is simpler to merge, review and may extend it later to this complex configuration.

@florianfesti
Copy link
Owner

Ok, I rebased this and re-run the pre-commit on the code. I now have one failure (see checks). Looks like my local version sorts the imports differently than in the CI. Any idea on how to get rid of that issue?

@Rotzbua
Copy link
Contributor Author

Rotzbua commented Mar 19, 2024

@florianfesti did you run it at least twice because of the amount of changes?

@florianfesti florianfesti marked this pull request as ready for review March 22, 2024 21:04
@florianfesti florianfesti merged commit ea7b482 into florianfesti:master Mar 22, 2024
2 checks passed
@florianfesti
Copy link
Owner

No, my local version seems to order that one import differently. I'll merge anyway.May be I disable the import ordering later on if that creates too much of an issue.

@Rotzbua
Copy link
Contributor Author

Rotzbua commented Mar 25, 2024

Thanks for the feedback. I will try to find the reason for this behavour.

@Rotzbua Rotzbua deleted the add_precommit branch March 25, 2024 15:42
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.

2 participants