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

Pre-commit hook suggestions #604

Draft
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

SmileyChris
Copy link
Contributor

@SmileyChris SmileyChris commented May 24, 2024

Description

As discussed in #600, here are my suggestions on the pre-commit changes.

  • renaming towncrier-check -> towncrier-draft. It is set to trigger if any text files have changed.
  • adding towncrier-checks (which does towncrier check, but keeping the name different so it doesn't clash with the renamed previous hook). It is set to always trigger before git push.
  • update towncrier-update (this does towncrier build but I don't think the name needs changing) to only trigger by the manual stage by default (as I doubt you'd want to do this every time you commit...)

@adiroiban
Copy link
Member

Thanks for the PR.

I have never used to pre-commit hooks for my projects.

I have custom GitHub actions steps to call towncrier as part of a PR branch protection checks and I always do both check and draft.

And draft is piped to GitHub Action summary , for easy review... just like we have for towncrier itself

https://github.com/twisted/towncrier/actions/runs/9216320582#summary-25356369369

I am -1 for having a separate hook for check and draft

Don't worry to much. As I mentioned, I am not using pre-commit hooks... so I don't care too much.

But I think that it would help to get feedback for more people.
So maybe, just as a review to @twisted/towncrier-contributors


My only important comment is about backward compatibility.

I see the test pass. So the change should be ok.

I think that for backward compatibility testing, it's important to keep this check, pinned at the current version

- repo: https://github.com/twisted/towncrier
rev: 23.11.0
hooks:
- id: towncrier-check


I guess that this PR will update .pre-commit-config.yaml to add the news hooks.


I have never used towncrier-update .

Does this hook make sense ?

@SmileyChris
Copy link
Contributor Author

SmileyChris commented May 29, 2024

Personally I think the only sensible hook is one that does the equivalent of towncrier check. It makes sense to have a pre-merge git hook to check for the presence of a news fragment.

My suggestions here were to add that one, but keep the others and just better document them (and have more sensible naming and better defaults). The only other related change that may make sense is keeping towncrier-check around and just have it raise a deprecation warning.

If we think there's no use to them, I'm fine "burning the chaff" too.

@adiroiban
Copy link
Member

It ok to have them. No problem.

I am not a big user of pre-commit , so anything is ok for me.

I am not -1.

We can have this PR in review for one week, and if you are happy with the hooks and nobody is agains the changes, we can merge it.

Thanks

@SmileyChris SmileyChris mentioned this pull request Jun 14, 2024
@dorschw
Copy link
Contributor

dorschw commented Jul 28, 2024

Hey, this can be useful for us, any ETA for a merge? (I saw the discussion)

@adiroiban
Copy link
Member

Hi @dorschw this PR is still draft.... and currenlty inactive.

If you have time, you can try to create a PR with the kind of pre-commit hooks that you would like to have in towncrier.

Cheers

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.

3 participants