Skip to content

refactor: set default_install_hook_types #697

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

Merged
merged 4 commits into from
Apr 23, 2023

Conversation

12rambau
Copy link
Contributor

@12rambau 12rambau commented Apr 5, 2023

Description

Since pre-commit 2.18.0, it is possible to tell pre-commit which hook to install directly from the .pre-commit-config.yaml file. Thus I update the said file to apply the hooks according to the docs: pre-commit install -t pre-commit -t pre-push -t commit-msg. I also updated the pre-commit version to make sure this parameter is available and finally updated the contributing documentation to make sure people simply use the pre-commit installcommand.

bonus: there was a typo that I corrected (it's "stageS")

Checklist

  • Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the change

@12rambau 12rambau changed the title Pre commit refactor: set default_install_hook_types Apr 5, 2023
@12rambau 12rambau marked this pull request as ready for review April 5, 2023 15:49
@12rambau
Copy link
Contributor Author

12rambau commented Apr 5, 2023

it needs #607 to be solved first as this requires at least 3.7

@woile
Copy link
Member

woile commented Apr 6, 2023

Thanks for the PR! Could you point this PR to v3 branch? because of the 3.7 dep

@12rambau 12rambau changed the base branch from master to v3 April 7, 2023 07:41
@12rambau
Copy link
Contributor Author

12rambau commented Apr 7, 2023

Thanks for the PR! Could you point this PR to v3 branch? because of the 3.7 dep

done

@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (1a1e73b) 97.37% compared to head (1153908) 97.42%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #697      +/-   ##
==========================================
+ Coverage   97.37%   97.42%   +0.04%     
==========================================
  Files          42       42              
  Lines        2022     2022              
==========================================
+ Hits         1969     1970       +1     
+ Misses         53       52       -1     
Flag Coverage Δ
unittests 97.42% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
commitizen/__version__.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@woile
Copy link
Member

woile commented Apr 19, 2023

Would you mind rebasing on top of v3? Thanks!

@12rambau
Copy link
Contributor Author

isn't it already the case ?

@woile
Copy link
Member

woile commented Apr 19, 2023

No, v3 was merged, you can see all kind of unrelated commits in the list.

You can follow this instructions to rebase: #686 (comment)

@12rambau
Copy link
Contributor Author

12rambau commented Apr 19, 2023

sorry I'm used to work in repositories where we squash merge PR so this is not considered as an issue.

@12rambau
Copy link
Contributor Author

not sure I did the right thing but THB it's a super small modifcation. merge squash it it wil make everyone's life easier.

Also the PR will look strange but upon merging, git resolves the duplicates commits (as they have the same hash) so why rebasing ?

@woile woile requested a review from Lee-W April 23, 2023 05:23
@Lee-W Lee-W changed the base branch from v3 to master April 23, 2023 06:24
@Lee-W Lee-W added the pr-status: ready-to-merge almost ready to merge. just keep it for a few days for others to check label Apr 23, 2023
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

Thanks @12rambau . This looks great!

@woile I'm planning on merging this one today.

@woile woile merged commit bcddd8d into commitizen-tools:master Apr 23, 2023
@12rambau 12rambau deleted the pre-commit branch April 23, 2023 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: ready-to-merge almost ready to merge. just keep it for a few days for others to check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants