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

The towncrier-check pre-commit hook doesn't actually run towncrier check #600

Open
SmileyChris opened this issue May 21, 2024 · 5 comments

Comments

@SmileyChris
Copy link
Contributor

This seems an unnatural correlation and I'm not sure what the purpose of the current hook (towncrier --draft) would really even be!

Also, it would make sense that this always runs, not just when a file is found in newsfragments/ since the check is supposed to be checking whether or not there are actually changelog updates, correct?

@adiroiban
Copy link
Member

I am using towncrier --draft from my project as part of CI to make sure there is no invalid RST syntax in those files.

@adiroiban
Copy link
Member

I am not using pre-commit hooks for my projects ... so I don't know how and why they should be used.

I think that we should start fixing this issue by documenting the pre-commit hooks so that we have the information of what we want to achieve with the hook


Things that I check for a branch / PR:

  • make sure every branch/PR has a newsfragment file
  • make sure the compiled news file is valid RST
  • for a release PR make sure there are no newsfragment files

Things that are needed and not yet implemented

@SmileyChris
Copy link
Contributor Author

pre-commit is useful to enforce standards. Since it needs active buy-in from your developers to actually use it, it usually lives as a git action too to actually enforce it :)

From the things you check for, I'd say we should have different hooks that do each of those:

  • check to actually check for newsfragments (since we have a towncrier command of the same name).
    • default stage should be pre-push
    • should use always_run
  • validate which ensures the validity of the compiled news file (and maybe fragments too) This should be an extension of towncrier check tbh.
    • probably types: ['text']
  • I wonder if the release PR check should also be a new argument to towncrier check ensuring there are no loose fragments? This probably isn't that useful as a hook, since you can't tie hooks to a specific branch merge.

@adiroiban
Copy link
Member

So far the approach is to have a single towncrier check that does everything.

towncrier check should be smart enough to detect when we have a normal branch, a release branch or a revert branch, and do its magic.

@SmileyChris
Copy link
Contributor Author

Since this discussion was getting a bit esoteric, I thought it would be good to actually just suggest the changes to the current hooks. The current towncrier-check hook is confusing since all it does is build --draft when we have a check action.

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

No branches or pull requests

2 participants