twisted / towncrier

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

Issue number incorrectly inferred when hash is all digits #584

Closed jaraco closed 2 months ago

jaraco commented 2 months ago

I've been using towncrier with mostly default configuration to generate news entries. I'll sometimes give a filename with an issue number ({issue}.{scope}.rst) and other times use +.{scope}.rst, and that works well. When I supply an issue number, it's included in the changelog and when I don't, it's omitted.

However, today when using towncrier 23.11, my expectation was violated when the generated hash was all digits:

 backports.tarfile main @ towncrier create -c "Initial release based on tarfile from Python 3.12.2." +.removal.rst
Created news fragment at /Users/jaraco/code/jaraco/backports.tarfile/newsfragments/+96333363.removal.rst
 backports.tarfile main @ git add newsfragments; git commit -a -m "Add news fragment."
[main ec51166] Add news fragment.
 1 file changed, 1 insertion(+)
 create mode 100644 newsfragments/+96333363.removal.rst
 backports.tarfile main @ tox -e finalize
finalize: install_deps> python -I -m pip install 'jaraco.develop>=7.23' towncrier
finalize: commands[0]> python -m jaraco.develop.finalize
Loading template...
/Users/jaraco/code/jaraco/backports.tarfile/.tox/finalize/lib/python3.12/site-packages/towncrier/build.py:169: EncodingWarning: 'encoding' argument not specified
  resources.files(config.template[0]).joinpath(config.template[1]).read_text()
Finding news fragments...
Rendering news fragments...
Writing to newsfile...
Staging newsfile...
Removing the following files:
/Users/jaraco/code/jaraco/backports.tarfile/newsfragments/+96333363.removal.rst
Removing news fragments...
Done!
[main 786b22e] Finalize
 2 files changed, 7 insertions(+), 1 deletion(-)
 delete mode 100644 newsfragments/+96333363.removal.rst
  finalize: OK (5.36=setup[4.58]+cmd[0.78] seconds)
  congratulations :) (5.42 seconds)
 backports.tarfile main @ head NEWS.rst
v1.0.0
======

Deprecations and Removals
-------------------------

- Initial release based on tarfile from Python 3.12.2. (#96333363)

It seems that if the hex hash happens not to have any digits greater than 9, the hash is inferred to be an issue number. That feels like a bug, since the same behavior would not produce an issue number if there were hex digits greater than 9. Perhaps towncrier should provide a way to distinguish between a hash and a typical issue number.

adiroiban commented 2 months ago

Many thanks Jason for the report.

It looks like a bug to me.

I could not find the documentation for this +.scope.rst functionality.

I see this functionality was added here https://github.com/twisted/towncrier/pull/428

with the code

filename = f"{orphan_prefix}{os.urandom(4).hex()}{filename[1:]}"

I saw there was another bugfix in https://github.com/twisted/towncrier/pull/468


I think that the fix is to use either a random function that only generates ASCII letters or prefix/suffix the random number with a fixed ASCII letter:

-filename = f"{orphan_prefix}{os.urandom(4).hex()}{filename[1:]}"
+filename = f"{orphan_prefix}a{os.urandom(4).hex()}{filename[1:]}"
webknjaz commented 2 months ago

(more feedback, kinda related)

I think that the fix is to use either a random function that only generates ASCII letters or prefix/suffix the random number with a fixed ASCII letter:

I think, it would be useful to provide relevant metadata in the Jinja2 context as a flag or something along those lines, so that the templates could decide whether to show something. Currently, the leading + seems to be stripped off with the rest provided as an identifier, like regular numbers, making it impossible to distinguish whether a given entry was orphan or just has a different reference format.

I started migrating to a custom Towncrier template in many of my projects recently, where I implemented the ability to reference issues/PR, commits and arbitrary references separately: https://github.com/cherrypy/cheroot/blob/3591a1c/docs/changelog-fragments.d/.towncrier-template.rst.j2#L65-L69. Having some kind of a flag or that leading plus preserved in the template context would've been useful for having different logic for orphan entries. Currently, the orphans would fall under one of those other categories detected by in-template logic.

SmileyChris commented 2 months ago

@jaraco should probably enter the lottery, there was only a 0.0035% chance of that!

SmileyChris commented 2 months ago

I could not find the documentation for this +.scope.rst functionality.

Added for you in #589

SmileyChris commented 2 months ago

I think, it would be useful to provide relevant metadata in the Jinja2 context as a flag or something along those lines, so that the templates could decide whether to show something. Currently, the leading + seems to be stripped off with the rest provided as an identifier, like regular numbers, making it impossible to distinguish whether a given entry was orphan or just has a different reference format.

The stripping of the '+' was a bug being fixed in #588. The correct behaviour is that fragments starting with the orphan character should have no issue, so while it's implicit, you can distinguish orphans if their fragment's issue variable is falsey.

jaraco commented 2 months ago

@jaraco should probably enter the lottery, there was only a 0.0035% chance of that!

Is that all? By my estimation, the probability is 2.3%. There's 10/16 chance that any given digit is 0-9 and there are 8 digits, so the probability that they're all digits is (10/16)^8:

>>> (10/16) ** 8
0.023283064365386963

That's more in line with my experience, as I've encountered this more than once and I've probably used +.scope.rst maybe 100 times.

jaraco commented 2 months ago

Thanks everyone for jumping on this!