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

[SmileyChris]

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]

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]

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

• add support for reversal branches #598

[SmileyChris]

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]

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]

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.

[krpatter-intc]

I ran across this today. You can make it run the check by changing the entry but I think the underlying issue is that towncrier check will always fail because it checks the files already committed to the two branches - and instead needs to check

(branch + staged files) vs. (check banch)

So I think for this to work as a pre-commit hook, we first need towncrier check to take in to account staged files.

Was that the previous findings as well?

[adiroiban]

Hi Kevin. Thanks for the feedback

I don't have strong oppinions for a pre-commit hook.

There is this PR https://github.com/twisted/towncrier/pull/604

---

As far as I know towncrier check is designed as a post-commit task.

But I think that it makes sense to add a flag like towncrier check --staged to include both branch and the staged files in the list of checks

I have happy to review and approve a PR for this.

But I think that updating towncrier check to include the staged files should have its separate GitHub issue.

I am ok with towncrier check --staged or towncrie pre-commit ... anything that would work for you.