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

Check for invalid filenames in changes.d/ #619

Closed
MetRonnie opened this issue Jun 28, 2024 · 9 comments · Fixed by #622
Closed

Check for invalid filenames in changes.d/ #619

MetRonnie opened this issue Jun 28, 2024 · 9 comments · Fixed by #622

Comments

@MetRonnie
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

MetRonnie commented Jul 1, 2024

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

@Avasam
Copy link

Avasam commented Jul 14, 2024

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

  • it has 3 segments (edit: I didn't know about postfix, so 3-4 segments)
  • the first starts with + or is a positive number
  • the second is a configured news type (prevent fragments being left behind accidentally because of a typo)
  • (edit: an optional part that should be a positive number)
  • the third is a supported file extension (.md, .rst, etc., depending on what's configured)
  • And finally, a list of files to ignore as part of this validation. Like a README or .gitignore, that serve either as documentation on how to write your changes for a project, or simply exists for the sake of always keeping the folder in git. For example: https://github.com/pypa/setuptools/tree/main/newsfragments

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
Copy link
Member

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

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 a pull request may close this issue.

3 participants