Repository: twisted/towncrier
Pull request #608 (2024-06-05 03:29:54)

Refactor issue_key function to sort issues in a human-friendly way
# Description

In the issue key sorting method, there's this comment:
```python
        # Maybe we should sniff strings like "gh-10" -> (10, "gh-10")?
```

Let's do that.

To be more backwards compatible, we'll continue putting just plain numbers last. In reality, it's very unlikely a project is mixing issue types, but if they are, then they'll be grouped:

```python
>>> sorted(["2", "#11", "#3", "gh-10", "gh-4", "omega", "alpha"], key=issue_key)
['alpha', 'omega', '#3', '#11', 'gh-4', 'gh-10', '2']
```

# Checklist
<!-- add a "X" inside the brackets to confirm -->
* [x] Make sure changes are covered by existing or new tests.
* [x] For at least one Python version, make sure local test run is green.
* [x] Create a file in `src/towncrier/newsfragments/`. Describe your
  change and include important information. Your change will be included in the public release notes.
* [x] Make sure all GitHub Actions checks are green (they are automatically checking all of the above).

SmileyChris: > I am not sure what an "issue" is: news fragment vs ticket ID

@adiroiban I actually decided to switch to issue rather than ticket. My logic is it's the only term actually used in code and changing it for consistency would be backwards incompatible.

We use the `issue_format` configuration option and the `issues_by_category` context argument when rendering.

Even though it's not your preference, are you ok with using "issue" across the board instead of ticket?
adiroiban: Thanks for the update

> Even though it's not your preference, are you ok with using "issue" across the board instead of ticket?

Sure. No problem.

Your suggestion is much bettter.

Happy to see some improvements here

Thanks


Patch:
diff --git a/NEWS.rst b/NEWS.rst
index 70db873..23215d5 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -15,9 +15,9 @@ Bugfixes
 --------
 
 - ``build`` now treats a missing fragments directory the same as an empty one, consistent with other operations. (`#538 <https://github.com/twisted/towncrier/issues/538>`_)
-- 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`. (`#562 <https://github.com/twisted/towncrier/issues/562>`_)
-- 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. (`#562 <https://github.com/twisted/towncrier/issues/562>`_)
+- Fragments with filenames like `fix-1.2.3.feature` are now associated with the issue `fix-1.2.3`.
+  In previous versions they were incorrectly associated to issue `3`. (`#562 <https://github.com/twisted/towncrier/issues/562>`_)
+- Orphan newsfragments containing numeric values are no longer accidentally associated to issues. In previous versions the orphan marker was ignored and the newsfragment was associated to an issue having the last numerical value from the filename. (`#562 <https://github.com/twisted/towncrier/issues/562>`_)
 
 
 Misc
@@ -248,7 +248,7 @@ towncrier 21.3.0.rc1 (2021-03-21)
 Features
 --------
 
-- Ticket number from file names will be stripped down to avoid ticket links such as ``#007``. (`#126 <https://github.com/twisted/towncrier/issues/126>`_)
+- Issue number from file names will be stripped down to avoid issue links such as ``#007``. (`#126 <https://github.com/twisted/towncrier/issues/126>`_)
 - Allow definition of the project ``version`` and ``name`` in the configuration file.
   This allows use of towncrier seamlessly with non-Python projects. (`#165 <https://github.com/twisted/towncrier/issues/165>`_)
 - Improve news fragment file name parsing to allow using file names like
diff --git a/README.rst b/README.rst
index da2250a..f6cda3e 100644
--- a/README.rst
+++ b/README.rst
@@ -30,7 +30,7 @@ Philosophy
 That is, by duplicating what has changed from the "developer log" (which may contain complex information about the original issue, how it was fixed, who authored the fix, and who reviewed the fix) into a "news fragment" (a small file containing just enough information to be useful to end users), ``towncrier`` can produce a digest of the changes which is valuable to those who may wish to use the software.
 These fragments are also commonly called "topfiles" or "newsfiles".
 
-``towncrier`` works best in a development system where all merges involve closing a ticket.
+``towncrier`` works best in a development system where all merges involve closing an issue.
 
 To get started, check out our `tutorial <https://towncrier.readthedocs.io/en/latest/tutorial.html>`_!
 
diff --git a/docs/customization/newsfile.rst b/docs/customization/newsfile.rst
index 67f3df4..03b97a3 100644
--- a/docs/customization/newsfile.rst
+++ b/docs/customization/newsfile.rst
@@ -4,7 +4,7 @@ Customizing the News File Output
 Adding Content Above ``towncrier``
 ----------------------------------
 
-If you wish to have content at the top of the news file (for example, to say where you can find the tickets), you can use a special rST comment to tell ``towncrier`` to only update after it.
+If you wish to have content at the top of the news file (for example, to say where you can find the issues), you can use a special rST comment to tell ``towncrier`` to only update after it.
 In your existing news file (e.g. ``NEWS.rst``), add the following line above where you want ``towncrier`` to put content:
 
 .. code-block:: restructuredtext
diff --git a/docs/tutorial.rst b/docs/tutorial.rst
index 258a132..70b9c41 100644
--- a/docs/tutorial.rst
+++ b/docs/tutorial.rst
@@ -89,9 +89,9 @@ The five default types are:
 - ``bugfix``: Signifying a bug fix.
 - ``doc``: Signifying a documentation improvement.
 - ``removal``: Signifying a deprecation or removal of public API.
-- ``misc``: A ticket has been closed, but it is not of interest to users.
+- ``misc``: An issue has been closed, but it is not of interest to users.
 
-When you create a news fragment, the filename consists of the ticket ID (or some other unique identifier) as well as the 'type'.
+When you create a news fragment, the filename consists of the issue/ticket ID (or some other unique identifier) as well as the 'type'.
 ``towncrier`` does not care about the fragment's suffix.
 
 You can create those fragments either by hand, or using the ``towncrier create`` command.
@@ -99,14 +99,14 @@ Let's create some example news fragments to demonstrate::
 
    $ echo 'Fixed a thing!' > src/myproject/newsfragments/1234.bugfix
    $ towncrier create --content 'Can also be ``rst`` as well!' 3456.doc.rst
-   # You can associate multiple ticket numbers with a news fragment by giving them the same contents.
+   # You can associate multiple issue numbers with a news fragment by giving them the same contents.
    $ towncrier create --content 'Can also be ``rst`` as well!' 7890.doc.rst
    $ echo 'The final part is ignored, so set it to whatever you want.' > src/myproject/newsfragments/8765.removal.txt
    $ echo 'misc is special, and does not put the contents of the file in the newsfile.' > src/myproject/newsfragments/1.misc
    $ towncrier create --edit 2.misc.rst  # starts an editor
-   $ towncrier create -c "Orphan fragments have no ticket ID." +random.bugfix.rst
+   $ towncrier create -c "Orphan fragments have no issue ID." +random.bugfix.rst
 
-For orphan news fragments (those that don't need to be linked to any ticket ID or other identifier), start the file name with ``+``.
+For orphan news fragments (those that don't need to be linked to any issue ID or other identifier), start the file name with ``+``.
 The content will still be included in the release notes, at the end of the category corresponding to the file extension::
 
    $ echo 'Fixed an unreported thing!' > src/myproject/newsfragments/+anything.bugfix
@@ -132,7 +132,7 @@ You should get an output similar to this::
    --------
 
    - Fixed a thing! (#1234)
-   - Orphan fragments have no ticket ID.
+   - Orphan fragments have no issue ID.
 
 
    Improved Documentation
@@ -167,7 +167,7 @@ To produce the news file for real, run::
 This command will remove the news files (with ``git rm``) and append the built news to the filename specified in ``pyproject.toml``, and then stage the news file changes (with ``git add``).
 It leaves committing the changes up to the user.
 
-If you wish to have content at the top of the news file (for example, to say where you can find the tickets), put your text above a rST comment that says::
+If you wish to have content at the top of the news file (for example, to say where you can find the issues), put your text above a rST comment that says::
 
   .. towncrier release notes start
 
diff --git a/src/towncrier/_builder.py b/src/towncrier/_builder.py
index 3a4591d..751d4b1 100644
--- a/src/towncrier/_builder.py
+++ b/src/towncrier/_builder.py
@@ -5,16 +5,17 @@
 from __future__ import annotations
 
 import os
+import re
 import textwrap
 
 from collections import defaultdict
 from pathlib import Path
-from typing import Any, DefaultDict, Iterable, Iterator, Mapping, Sequence
+from typing import Any, DefaultDict, Iterable, Iterator, Mapping, NamedTuple, Sequence
 
 from jinja2 import Template
 
 
-# Returns ticket, category and counter or (None, None, None) if the basename
+# Returns issue, 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(
     basename: str, frag_type_names: Iterable[str]
@@ -33,21 +34,21 @@ def parse_newfragment_basename(
         if parts[i] in frag_type_names:
             # Current part is a valid category according to given definitions.
             category = parts[i]
-            # Use all previous parts as the ticket number.
+            # Use all previous parts as the issue 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
+            # something-cool.feature.ext for projects that don't use issue
             # numbers in news fragment names.
-            ticket = ".".join(parts[0:i]).strip()
-            # If the ticket is an integer, remove any leading zeros (to resolve
+            issue = ".".join(parts[0:i]).strip()
+            # If the issue is an integer, remove any leading zeros (to resolve
             # issue #126).
-            if ticket.isdigit():
-                ticket = str(int(ticket))
+            if issue.isdigit():
+                issue = str(int(issue))
             counter = 0
             # Use the following part as the counter if it exists and is a valid
             # digit.
             if len(parts) > (i + 1) and parts[i + 1].isdigit():
                 counter = int(parts[i + 1])
-            return ticket, category, counter
+            return issue, category, counter
     else:
         # No valid category found.
         return invalid
@@ -97,15 +98,15 @@ def find_fragments(
         file_content = {}
 
         for basename in files:
-            ticket, category, counter = parse_newfragment_basename(
+            issue, category, counter = parse_newfragment_basename(
                 basename, frag_type_names
             )
             if category is None:
                 continue
-            assert ticket is not None
+            assert issue is not None
             assert counter is not None
-            if orphan_prefix and ticket.startswith(orphan_prefix):
-                ticket = ""
+            if orphan_prefix and issue.startswith(orphan_prefix):
+                issue = ""
                 # Use and increment the orphan news fragment counter.
                 counter = orphan_fragment_counter[category]
                 orphan_fragment_counter[category] += 1
@@ -114,13 +115,13 @@ def find_fragments(
             fragment_filenames.append(full_filename)
             data = Path(full_filename).read_text(encoding="utf-8", errors="replace")
 
-            if (ticket, category, counter) in file_content:
+            if (issue, category, counter) in file_content:
                 raise ValueError(
                     "multiple files for {}.{} in {}".format(
-                        ticket, category, section_dir
+                        issue, category, section_dir
                     )
                 )
-            file_content[ticket, category, counter] = data
+            file_content[issue, category, counter] = data
 
         content[key] = file_content
 
@@ -153,7 +154,7 @@ def split_fragments(
     for section_name, section_fragments in fragments.items():
         section: dict[str, dict[str, list[str]]] = {}
 
-        for (ticket, category, counter), content in section_fragments.items():
+        for (issue, category, counter), content in section_fragments.items():
             if all_bullets:
                 # By default all fragmetns are append by "-" automatically,
                 # and need to be indented because of that.
@@ -168,30 +169,57 @@ def split_fragments(
 
             texts = section.setdefault(category, {})
 
-            tickets = texts.setdefault(content, [])
-            if ticket:
-                # Only add the ticket if we have one (it can be blank for orphan news
+            issues = texts.setdefault(content, [])
+            if issue:
+                # Only add the issue if we have one (it can be blank for orphan news
                 # fragments).
-                tickets.append(ticket)
-                tickets.sort()
+                issues.append(issue)
+                issues.sort()
 
         output[section_name] = section
 
     return output
 
 
-def issue_key(issue: str) -> tuple[int, str]:
-    # We want integer issues to sort as integers, and we also want string
-    # issues to sort as strings. We arbitrarily put string issues before
-    # integer issues (hopefully no-one uses both at once).
-    try:
-        return (int(issue), "")
-    except Exception:
-        # Maybe we should sniff strings like "gh-10" -> (10, "gh-10")?
-        return (-1, issue)
+class IssueParts(NamedTuple):
+    is_digit: bool
+    has_digit: bool
+    non_digit_part: str
+    number: int
 
 
-def entry_key(entry: tuple[str, Sequence[str]]) -> tuple[str, list[tuple[int, str]]]:
+def issue_key(issue: str) -> IssueParts:
+    """
+    Used to sort the issue ID inside a news fragment in a human-friendly way.
+
+    Issue IDs are grouped by their non-integer part, then sorted by their integer part.
+
+    For backwards compatible consistency, issues without no number are sorted first and
+    digit only issues are sorted last.
+
+    For example::
+
+    >>> sorted(["2", "#11", "#3", "gh-10", "gh-4", "omega", "alpha"], key=issue_key)
+    ['alpha', 'omega', '#3', '#11', 'gh-4', 'gh-10', '2']
+    """
+    if issue.isdigit():
+        return IssueParts(
+            is_digit=True, has_digit=True, non_digit_part="", number=int(issue)
+        )
+    match = re.search(r"\d+", issue)
+    if not match:
+        return IssueParts(
+            is_digit=False, has_digit=False, non_digit_part=issue, number=-1
+        )
+    return IssueParts(
+        is_digit=False,
+        has_digit=True,
+        non_digit_part=issue[: match.start()] + issue[match.end() :],
+        number=int(match.group()),
+    )
+
+
+def entry_key(entry: tuple[str, Sequence[str]]) -> tuple[str, list[IssueParts]]:
     content, issues = entry
     # Orphan news fragments (those without any issues) should sort last by content.
     return "" if issues else content, [issue_key(issue) for issue in issues]
diff --git a/src/towncrier/create.py b/src/towncrier/create.py
index 77433fc..39fddec 100644
--- a/src/towncrier/create.py
+++ b/src/towncrier/create.py
@@ -70,7 +70,7 @@ def _main(
     * .bugfix - a bug fix
     * .doc - a documentation improvement,
     * .removal - a deprecation or removal of public API,
-    * .misc - a ticket has been closed, but it is not of interest to users.
+    * .misc - an issue has been closed, but it is not of interest to users.
 
     If the FILENAME base is just '+' (to create a fragment not tied to an
     issue), it will be appended with a random hex string.
diff --git a/src/towncrier/newsfragments/608.feature.rst b/src/towncrier/newsfragments/608.feature.rst
new file mode 100644
index 0000000..4cccc12
--- /dev/null
+++ b/src/towncrier/newsfragments/608.feature.rst
@@ -0,0 +1,8 @@
+News fragments are now sorted by issue number even if they have non-digit characters.
+For example::
+
+    - some issue (gh-3, gh-10)
+    - another issue (gh-4)
+    - yet another issue (gh-11)
+
+The sorting algorithm groups the issues first by non-text characters and then by number.


Test patch:
diff --git a/src/towncrier/test/test_build.py b/src/towncrier/test/test_build.py
index 23cfb86..85a21bd 100644
--- a/src/towncrier/test/test_build.py
+++ b/src/towncrier/test/test_build.py
@@ -914,7 +914,7 @@ class TestCli(TestCase):
         """
         When all_bullets is false, subsequent lines are not indented.
 
-        The automatic ticket number inserted by towncrier will align with the
+        The automatic issue number inserted by towncrier will align with the
         manual bullet.
         """
         os.mkdir("newsfragments")
diff --git a/src/towncrier/test/test_builder.py b/src/towncrier/test/test_builder.py
index 9608e0a..1213e1a 100644
--- a/src/towncrier/test/test_builder.py
+++ b/src/towncrier/test/test_builder.py
@@ -1,9 +1,11 @@
 # Copyright (c) Povilas Kanapickas, 2019
 # See LICENSE for details.
 
+from textwrap import dedent
+
 from twisted.trial.unittest import TestCase
 
-from .._builder import parse_newfragment_basename
+from .._builder import parse_newfragment_basename, render_fragments
 
 
 class TestParseNewsfragmentBasename(TestCase):
@@ -42,35 +44,35 @@ class TestParseNewsfragmentBasename(TestCase):
             ("123", "feature", 0),
         )
 
-    def test_non_numeric_ticket(self):
+    def test_non_numeric_issue(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):
+    def test_non_numeric_issue_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):
+    def test_dots_in_issue_name(self):
         """Non-numeric issue identifiers are preserved verbatim."""
         self.assertEqual(
             parse_newfragment_basename("baz.1.2.feature", ["feature"]),
             ("baz.1.2", "feature", 0),
         )
 
-    def test_dots_in_ticket_name_invalid_category(self):
+    def test_dots_in_issue_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):
+    def test_dots_in_issue_name_and_counter(self):
         """Non-numeric issue identifiers are preserved verbatim."""
         self.assertEqual(
             parse_newfragment_basename("baz.1.2.feature.3", ["feature"]),
@@ -79,7 +81,7 @@ class TestParseNewsfragmentBasename(TestCase):
 
     def test_strip(self):
         """Leading spaces and subsequent leading zeros are stripped
-        when parsing newsfragment names into ticket numbers etc.
+        when parsing newsfragment names into issue numbers etc.
         """
         self.assertEqual(
             parse_newfragment_basename("  007.feature", ["feature"]),
@@ -88,7 +90,7 @@ class TestParseNewsfragmentBasename(TestCase):
 
     def test_strip_with_counter(self):
         """Leading spaces and subsequent leading zeros are stripped
-        when parsing newsfragment names into ticket numbers etc.
+        when parsing newsfragment names into issue numbers etc.
         """
         self.assertEqual(
             parse_newfragment_basename("  007.feature.3", ["feature"]),
@@ -132,3 +134,73 @@ class TestParseNewsfragmentBasename(TestCase):
             parse_newfragment_basename("+123.feature", ["feature"]),
             ("+123", "feature", 0),
         )
+
+
+class TestNewsFragmentsOrdering(TestCase):
+    """
+    Tests to ensure that issues are ordered correctly in the output.
+
+    This tests both ordering of issues within a fragment and ordering of
+    fragments within a section.
+    """
+
+    template = dedent(
+        """
+    {% for section_name, category in sections.items() %}
+    {% if section_name %}# {{ section_name }}{% endif %}
+    {%- for category_name, issues in category.items() %}
+    ## {{ category_name }}
+    {% for issue, numbers in issues.items() %}
+    - {{ issue }}{% if numbers %} ({{ numbers|join(', ') }}){% endif %}
+
+    {% endfor %}
+    {% endfor -%}
+    {% endfor -%}
+    """
+    )
+
+    def render(self, fragments):
+        return render_fragments(
+            template=self.template,
+            issue_format=None,
+            fragments=fragments,
+            definitions={},
+            underlines=[],
+            wrap=False,
+            versiondata={},
+        )
+
+    def test_ordering(self):
+        """
+        Issues are ordered first by the non-text component, then by their number.
+
+        For backwards compatibility, issues with no number are grouped first and issues
+        which are only a number are grouped last.
+
+        Orphan news fragments are always last, sorted by their text.
+        """
+        output = self.render(
+            {
+                "": {
+                    "feature": {
+                        "Added Cheese": ["10", "gh-25", "gh-3", "4"],
+                        "Added Fish": [],
+                        "Added Bread": [],
+                        "Added Milk": ["gh-1"],
+                        "Added Eggs": ["gh-2", "random"],
+                    }
+                }
+            },
+        )
+        # "Eggs" are first because they have an issue with no number, and the first
+        # issue for each fragment is what is used for sorting the overall list.
+        assert output == dedent(
+            """
+            ## feature
+            - Added Eggs (random, gh-2)
+            - Added Milk (gh-1)
+            - Added Cheese (gh-3, gh-25, #4, #10)
+            - Added Bread
+            - Added Fish
+"""
+        )