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

Fix orphan fragments with numbers #564

Merged
merged 2 commits into from
Nov 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
.vs/
.vscode
Justfile
*egg-info/
Copy link
Member

Choose a reason for hiding this comment

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

I guess we can have this. I have this rule in my global gitignore file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call.
Also this is a python project, so i'd expect that directory to exist eventually.

_trial_temp*/
apidocs/
dist/
Expand Down
14 changes: 5 additions & 9 deletions src/towncrier/_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,24 +32,20 @@ def parse_newfragment_basename(

if len(parts) == 1:
return invalid
if len(parts) == 2:
ticket, category = parts
ticket = strip_if_integer_string(ticket)
return (ticket, category, 0) if category in frag_type_names else invalid

# There are at least 3 parts. Search for a valid category from the second
# part onwards.
# There are at least 2 parts. Search for a valid category from the second
# part onwards starting at the back.
# The category is used as the reference point in the parts list to later
# infer the issue number and counter value.
for i in range(1, len(parts)):
for i in reversed(range(1, len(parts))):
if parts[i] in frag_type_names:
# Current part is a valid category according to given definitions.
category = parts[i]
# Use the previous part as the ticket number.
# Use all previous parts as the ticket number.
# 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(parts[i - 1])
ticket = strip_if_integer_string(".".join(parts[0:i]))
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/562.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Orphan newsfragments containing numeric values are no longer accidentally associated to tickets. In previous versions the orphan marker was ignored and the newsfragment was associated to a ticket having the last numerical value from the filename.
2 changes: 2 additions & 0 deletions src/towncrier/newsfragments/562.bugfix.1
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fragments with filenames like `fix-1.2.3.feature` are now associated with the ticket `fix-1.2.3`.
In previous versions they were incorrectly associated to ticket `3`.
21 changes: 19 additions & 2 deletions src/towncrier/test/test_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@ def _test_command(self, command):
f.write("Orphaned feature")
with open("foo/newsfragments/+xxx.feature", "w") as f:
f.write("Another orphaned feature")
with open("foo/newsfragments/+123_orphaned.feature", "w") as f:
f.write("An orphaned feature starting with a number")
with open("foo/newsfragments/+12.3_orphaned.feature", "w") as f:
f.write("An orphaned feature starting with a dotted number")
with open("foo/newsfragments/+orphaned_123.feature", "w") as f:
f.write("An orphaned feature ending with a number")
with open("foo/newsfragments/+orphaned_12.3.feature", "w") as f:
f.write("An orphaned feature ending with a dotted number")
# Towncrier ignores files that don't have a dot
with open("foo/newsfragments/README", "w") as f:
f.write("Blah blah")
Expand All @@ -52,7 +60,7 @@ def _test_command(self, command):

result = runner.invoke(command, ["--draft", "--date", "01-01-2001"])

self.assertEqual(0, result.exit_code)
self.assertEqual(0, result.exit_code, result.output)
self.assertEqual(
result.output,
dedent(
Expand All @@ -70,9 +78,13 @@ def _test_command(self, command):
--------

- Baz levitation (baz)
- Baz fix levitation (#2)
- Baz fix levitation (fix-1.2)
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'm not so sure about this one. But i'd argue, that is what was actually meant by:
https://github.com/twisted/towncrier/pull/564/files#diff-36db088409efe07783d49792a18cec6ae3778ace338bf1cd08daa8a3fce29e93R45-R47

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

Copy link
Member

Choose a reason for hiding this comment

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

I think that this need a better comment.

I don't really understant what is the use case here.

This is the bad part of bundling all kind of scenarios in a single test case.

            # Towncrier supports files that have a dot in the name of the
            # newsfragment
            with open("foo/newsfragments/fix-1.2.feature", "w") as f:
                f.write("Baz fix levitation")

since it has no orphan marker, I would consider it an error, and the file should be rejected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per my comment above, I think this snippet was parsed wrongly. But I cannot find anything in the docs about it the only reference is the comment (_builder.py):

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

And I do not think that in that case "3" was meant to be an issue number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I read the comment you cited as the "name of the newsfragment" being fix-1.2.

- Adds levitation (#123)
- Extends levitation (#124)
- An orphaned feature ending with a dotted number
- An orphaned feature ending with a number
- An orphaned feature starting with a dotted number
- An orphaned feature starting with a number
- Another orphaned feature
- Orphaned feature

Expand Down Expand Up @@ -405,6 +417,7 @@ def test_draft_no_date(self):
call(["git", "init"])
call(["git", "config", "user.name", "user"])
call(["git", "config", "user.email", "[email protected]"])
call(["git", "config", "commit.gpgSign", "false"])
Copy link
Member

@adiroiban adiroiban Nov 7, 2023

Choose a reason for hiding this comment

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

We might need to refactor this code into an helper function that does the git init for us.

I expect that this is needed in other tests.

We can leave this as it is for now.

There are numerous bad practices in the towncrier tests.
It would be nice to have them fix, but it should not be a burder for new contributors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this?

call(["git", "add", "."])
call(["git", "commit", "-m", "Initial Commit"])

Expand All @@ -429,6 +442,7 @@ def test_no_confirmation(self):
call(["git", "init"])
call(["git", "config", "user.name", "user"])
call(["git", "config", "user.email", "[email protected]"])
call(["git", "config", "commit.gpgSign", "false"])
call(["git", "add", "."])
call(["git", "commit", "-m", "Initial Commit"])

Expand Down Expand Up @@ -458,6 +472,7 @@ def test_keep_fragments(self, runner):
call(["git", "init"])
call(["git", "config", "user.name", "user"])
call(["git", "config", "user.email", "[email protected]"])
call(["git", "config", "commit.gpgSign", "false"])
call(["git", "add", "."])
call(["git", "commit", "-m", "Initial Commit"])

Expand Down Expand Up @@ -491,6 +506,7 @@ def test_yes_keep_error(self, runner):
call(["git", "init"])
call(["git", "config", "user.name", "user"])
call(["git", "config", "user.email", "[email protected]"])
call(["git", "config", "commit.gpgSign", "false"])
call(["git", "add", "."])
call(["git", "commit", "-m", "Initial Commit"])

Expand Down Expand Up @@ -519,6 +535,7 @@ def test_confirmation_says_no(self):
call(["git", "init"])
call(["git", "config", "user.name", "user"])
call(["git", "config", "user.email", "[email protected]"])
call(["git", "config", "commit.gpgSign", "false"])
call(["git", "add", "."])
call(["git", "commit", "-m", "Initial Commit"])

Expand Down
48 changes: 46 additions & 2 deletions src/towncrier/test/test_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,63 +8,73 @@

class TestParseNewsfragmentBasename(TestCase):
def test_simple(self):
"""<number>.<category> generates a counter value of 0."""
self.assertEqual(
parse_newfragment_basename("123.feature", ["feature"]),
("123", "feature", 0),
)

def test_invalid_category(self):
"""Files without a valid category are rejected."""
self.assertEqual(
parse_newfragment_basename("README.ext", ["feature"]),
(None, None, None),
)

def test_counter(self):
"""<number>.<category>.<counter> generates a custom counter value."""
self.assertEqual(
parse_newfragment_basename("123.feature.1", ["feature"]),
("123", "feature", 1),
)

def test_counter_with_extension(self):
"""File extensions are ignored."""
self.assertEqual(
parse_newfragment_basename("123.feature.1.ext", ["feature"]),
("123", "feature", 1),
)

def test_ignores_extension(self):
"""File extensions are ignored."""
self.assertEqual(
parse_newfragment_basename("123.feature.ext", ["feature"]),
("123", "feature", 0),
)

def test_non_numeric_ticket(self):
"""Non-numeric issue identifiers are preserved verbatim."""
self.assertEqual(
parse_newfragment_basename("baz.feature", ["feature"]),
("baz", "feature", 0),
)

def test_non_numeric_ticket_with_extension(self):
"""File extensions are ignored."""
self.assertEqual(
parse_newfragment_basename("baz.feature.ext", ["feature"]),
("baz", "feature", 0),
)

def test_dots_in_ticket_name(self):
"""Non-numeric issue identifiers are preserved verbatim."""
self.assertEqual(
parse_newfragment_basename("baz.1.2.feature", ["feature"]),
("2", "feature", 0),
("baz.1.2", "feature", 0),
adiroiban marked this conversation as resolved.
Show resolved Hide resolved
)

def test_dots_in_ticket_name_invalid_category(self):
"""Files without a valid category are rejected."""
self.assertEqual(
parse_newfragment_basename("baz.1.2.notfeature", ["feature"]),
(None, None, None),
)

def test_dots_in_ticket_name_and_counter(self):
"""Non-numeric issue identifiers are preserved verbatim."""
self.assertEqual(
parse_newfragment_basename("baz.1.2.feature.3", ["feature"]),
("2", "feature", 3),
("baz.1.2", "feature", 3),
)

def test_strip(self):
Expand All @@ -77,7 +87,41 @@ def test_strip(self):
)

def test_strip_with_counter(self):
"""Leading spaces and subsequent leading zeros are stripped
when parsing newsfragment names into ticket numbers etc.
"""
self.assertEqual(
parse_newfragment_basename(" 007.feature.3", ["feature"]),
("7", "feature", 3),
)

def test_orphan(self):
adiroiban marked this conversation as resolved.
Show resolved Hide resolved
"""Orphaned snippets must remain the orphan marker in the issue
identifier."""
self.assertEqual(
parse_newfragment_basename("+orphan.feature", ["feature"]),
("+orphan", "feature", 0),
)

def test_orphan_with_number(self):
"""Orphaned snippets can contain numbers in the identifier."""
self.assertEqual(
parse_newfragment_basename("+123_orphan.feature", ["feature"]),
("+123_orphan", "feature", 0),
)
self.assertEqual(
parse_newfragment_basename("+orphan_123.feature", ["feature"]),
("+orphan_123", "feature", 0),
)

def test_orphan_with_dotted_number(self):
"""Orphaned snippets can contain numbers with dots in the
identifier."""
self.assertEqual(
parse_newfragment_basename("+12.3_orphan.feature", ["feature"]),
("+12.3_orphan", "feature", 0),
)
self.assertEqual(
parse_newfragment_basename("+orphan_12.3.feature", ["feature"]),
("+orphan_12.3", "feature", 0),
)
Loading