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.

Sign up for GitHub

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

Jump to bottom

Fix incorrect handling of orphan fragment names consists of only digits #588

Merged
merged 5 commits into from
Apr 27, 2024
Merged

Fix incorrect handling of orphan fragment names consists of only digits #588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
26b1d04
Don't lose the flag at the start of decimal only orphans
SmileyChris Apr 24, 2024
d9b4139
Add news fragment
SmileyChris Apr 23, 2024
5d732ca
Reword news fragment to be less git-commenty
SmileyChris Apr 25, 2024
6bd47e4
mention why we're removing leading zeros
SmileyChris Apr 25, 2024
d941865
Merge branch 'trunk' into alphabetic-orphan-hash
adiroiban Apr 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions src/towncrier/_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,6 @@
from jinja2 import Template


def strip_if_integer_string(s: str) -> str:
try:
i = int(s)
except ValueError:
return s

return str(i)


# Returns ticket, category and counter or (None, None, None) if the basename
# could not be parsed or doesn't contain a valid category.
def parse_newfragment_basename(
Expand All @@ -45,7 +36,11 @@ def parse_newfragment_basename(
# NOTE: This allows news fragment names like fix-1.2.3.feature or
# something-cool.feature.ext for projects that don't use ticket
# numbers in news fragment names.
ticket = strip_if_integer_string(".".join(parts[0:i]))
ticket = ".".join(parts[0:i]).strip()
# If the ticket is an integer, remove any leading zeros (to resolve
# issue #126).
if ticket.isdigit():
SmileyChris marked this conversation as resolved.
Show resolved Hide resolved
ticket = str(int(ticket))
counter = 0
# Use the following part as the counter if it exists and is a valid
# digit.
Expand Down
1 change: 1 addition & 0 deletions src/towncrier/newsfragments/588.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Orphan news fragments, fragments not associated with an issue, consisting of only digits (e.g. '+12345678.feature') now retain their leading marker character.
7 changes: 7 additions & 0 deletions src/towncrier/test/test_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,3 +125,10 @@ def test_orphan_with_dotted_number(self):
parse_newfragment_basename("+orphan_12.3.feature", ["feature"]),
("+orphan_12.3", "feature", 0),
)

def test_orphan_all_digits(self):
"""Orphaned snippets can consist of only digits."""
self.assertEqual(
parse_newfragment_basename("+123.feature", ["feature"]),
("+123", "feature", 0),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add another non-orphan test with an identifier looking like a commit sha? In the templates I've started using recently, I try to output PRs/issues, commits and arbitrary identifiers separately: https://github.com/cherrypy/cheroot/blob/3591a1c/docs/changelog-fragments.d/.towncrier-template.rst.j2#L59-L63.

It'd be nice if that remained functional. Hence an explicit test...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that should be a separate PR -- that's an interesting functional test but not really tied to the changes being made here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I brought it up is that I wasn't sure if this PR influences it. So it's kinda related as a possible regression. But I understand that it may be non-atomic and separate otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback.

This test is named test_orphan_all_digits . Based on that, I think that +123 is ok.

I also think that using a git commit ref is also a valid test... but it should be a separate test.

)
Loading