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

Issue number incorrectly inferred when hash is all digits #584

Closed
jaraco opened this issue Apr 6, 2024 · 7 comments · Fixed by #588
Closed

Issue number incorrectly inferred when hash is all digits #584

jaraco opened this issue Apr 6, 2024 · 7 comments · Fixed by #588

Comments

@jaraco
Copy link
Contributor

jaraco commented Apr 6, 2024

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

adiroiban commented Apr 6, 2024

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 #428

with the code

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

I saw there was another bugfix in #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:]}"

jaraco referenced this issue in pypa/setuptools Apr 16, 2024
@webknjaz
Copy link
Contributor

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

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

SmileyChris added a commit to SmileyChris/towncrier that referenced this issue Apr 24, 2024
SmileyChris added a commit to SmileyChris/towncrier that referenced this issue Apr 24, 2024
@SmileyChris
Copy link
Contributor

SmileyChris commented Apr 24, 2024

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

Added for you in #589

@SmileyChris
Copy link
Contributor

SmileyChris commented Apr 24, 2024

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

jaraco commented Apr 24, 2024

@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.

adiroiban pushed a commit that referenced this issue Apr 26, 2024
* Add docs for CLI orphan fragments
Refs #584

* Add newsfragment

* Mention orphan fragments in doc

* Add a note about '+' to the create command's docstring
adiroiban added a commit that referenced this issue Apr 27, 2024
…ts (#588)

* Don't lose the flag at the start of decimal only orphans
Fixes #584

* Add news fragment

* Reword news fragment to be less git-commenty

* mention why we're removing leading zeros

---------

Co-authored-by: Adi Roiban <adiroiban@gmail.com>
@jaraco
Copy link
Contributor Author

jaraco commented Apr 27, 2024

Thanks everyone for jumping on this!

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.

4 participants