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

build: install all hooks type at once #258

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

12rambau
Copy link
Contributor

@12rambau 12rambau commented Apr 21, 2023

commitizen is a commit-msg hook, it won't be installed by default. Using the default_install_hook_types makes sure that all the hooks used in this file are installed when running:

pre-commit install

related to #257

commitizen is a commit-msg hook, it won't be installed by default. Using the `default_install_hook_types` makes sure that all the hooks used in this file are installed when running:

```
pre-commit install
```
@12rambau
Copy link
Contributor Author

@maartenbreddels note that the error raised from the CI is completely unraleted from my PR it's ready to be merged.

@12rambau
Copy link
Contributor Author

@mariobuikhuizen I've rebased on master as well and it runs like a charm

@12rambau
Copy link
Contributor Author

12rambau commented Sep 7, 2023

@mariobuikhuizen could you merge this one ? It's harmless for the rest of the code and it simply ensure that the commitizen hook is installed by default (commit-msh is not in the default list).

I'm constantly forgetting to add it when I work in a github devcontainer, it would make my life easier (and more transparent for someone that doesn't know pre-commits well)

@mariobuikhuizen
Copy link
Collaborator

I need to evaluate this tool before proceeding, but unfortunately, I can't prioritize it at the moment.

@12rambau
Copy link
Contributor Author

12rambau commented Nov 7, 2023

requesting review of this PR again: it changes nothing apart for pre-commit users. 2 hooks are already set-up but without this line, only one is installed by default when running "pre-commit install". As you have already completely adopted the conventional commit it will not even influence the way you code.

@mariobuikhuizen
Copy link
Collaborator

There are 3 issues I have with using this tool:

  1. You'll need to have node installed
  2. Not all commits in PR's will end up being merged because of squashing. So requiring strict rules for commits that probably get squashed is not ideal.
  3. It probably doesn't work with git rebase -i ...

Since you need it for the github devcontainer, would it help if you could configure you own custom dev container in .devcontainer/12rambau in this repo? See: https://docs.github.com/en/codespaces/setting-up-your-project-for-codespaces/adding-a-dev-container-configuration/introduction-to-dev-containers#creating-a-custom-dev-container-configuration

@12rambau
Copy link
Contributor Author

Then let's turn the table and drop commitizen pre-commit hook alltogether.

My point is if it's there it should be hard to miss to be useful. If you prefer not to use it, I'm nobody to enforce it but in that case let's drop it from pre-commit.

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