twisted / towncrier

Manage the release notes for your project.
https://towncrier.readthedocs.io
MIT License
781 stars 121 forks source link

Check for invalid filenames in `changes.d/` #619

Closed MetRonnie closed 2 months ago

MetRonnie commented 3 months ago

Although towncrier create enforces valid fragment file names, creating a fragment file manually can sometimes lead to mistakes in the file name. It would be useful to include in our CI a check that all files in changes.d/ have valid fragment file names (excluding changelog-template.jinja of course).

Note we don't use towncrier check as we don't create fragments for small or non-user-facing changes.

adiroiban commented 3 months ago

Hi Ronnie.

Thanks for the report.

I am not sure what is requested here.

Just asking :)

Can you please define what is considered a "valid fragment file name" ?

How else, other that via towncrier check are you expecting to have this implemented ?

Note we don't use towncrier check as we don't create fragments for small or non-user-facing changes.

You can create .misc or .ignore or '.hidden' fragments for small or non user facing changes. In this way, you explicitly acknowledge that this branch doesn't need public release notes.


I would say that you should start using towncrier check. It can help detect some errors in fragment file names.

MetRonnie commented 3 months ago

Can you please define what is considered a "valid fragment file name" ?

And invalid one would fail towncrier create, or would get ignored by towncrier build. E.g. fix.123.md instead of 123.fix.md

MetRonnie commented 3 months ago

You can create .misc or .ignore or '.hidden' fragments for small or non user facing changes. In this way, you explicitly acknowledge that this branch doesn't need public release notes.

(We have a checklist item for this. Creating fragments when they're not needed seems like more effort than it's worth)

adiroiban commented 3 months ago

How else, other that via towncrier check are you expecting to have this implemented ?

I am still not sure how you are expecting to have this implemented.

If you have time, consider creating a pull request with a suggestion on how to fix this issues.

As long as it doesn't breaks backward compatibility, it has good documentation and automated tests, we can have the PR merged.

MetRonnie commented 3 months ago

How else, other that via towncrier check are you expecting to have this implemented ?

Sorry, missed this question.

Maybe something like a towncrier check-existing command? It would run whatever pattern matching is done during towncrier create on each filename in the changes.d directory (or whatever directory the project has configured), excluding the jinja template file.

It would be nice to have, but I'm kind of limited on time to put into making a PR myself

adiroiban commented 3 months ago

Thanks for the clarification.

I am leaving this issue open.

Anyone else who is affected by this missing functionality can submit a PR with a fix for this issue.

I would still prever to have twisted check as the entry point for any validation jobs.

One option is to have this as a flag for twisted check and maybe as configuration file option.

towncrier check --ignore-missing

[tool.towncrier.check]
ignore_missing = True
MetRonnie commented 2 months ago

@adiroiban I had a quick go at this, turned out to be pretty easy. Let me know what you think: #622

Avasam commented 2 months ago

My need would be: For every file in the changes folder, check that:

In the mean time, I went with a custom python script to validate on the CI: https://github.com/Avasam/ptle-tools/blob/main/.github/workflows/verify_newsfragments.yaml https://github.com/Avasam/ptle-tools/blob/main/.github/verify_newsfragments.py

So did pyinstaller: https://github.com/bwoodsend/pyinstaller/blob/bootloader-build/.github/workflows/validate-new-news.yml https://github.com/bwoodsend/pyinstaller/blob/bootloader-build/scripts/verify-news-fragments.py

adiroiban commented 2 months ago

Thanks Avasam for the feedback.

We have PR #622 which tries to fix this issue.

If you have time, please consider reviewing the PR and leave your feedback for the proposed fix.

Thanks