From be32e6bc99c6dcf6e009d60445d02d1128be3471 Mon Sep 17 00:00:00 2001 From: Chris Beaven Date: Fri, 14 Jun 2024 09:56:11 +1200 Subject: [PATCH] Refactor issue_key function to sort issues in a human-friendly way (#608) * Refactor issue_key function to sort issues in a human-friendly way * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Rename newsfragment * Small improvement to test to show how text with numeric issues are sorted * Update src/towncrier/_builder.py docstring grammar Co-authored-by: Adi Roiban * clarify new behaviour in newsfragment * Add some docstrings/comments to tests * linelength fix * Clarify news fragments vs tickets Co-authored-by: Adi Roiban * Consistent use of "issue" rather than "ticket" * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * typo --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Adi Roiban --- NEWS.rst | 8 +- README.rst | 2 +- docs/customization/newsfile.rst | 2 +- docs/tutorial.rst | 14 ++-- src/towncrier/_builder.py | 92 ++++++++++++++------- src/towncrier/create.py | 2 +- src/towncrier/newsfragments/608.feature.rst | 8 ++ src/towncrier/test/test_build.py | 2 +- src/towncrier/test/test_builder.py | 88 ++++++++++++++++++-- 9 files changed, 163 insertions(+), 55 deletions(-) create mode 100644 src/towncrier/newsfragments/608.feature.rst diff --git a/NEWS.rst b/NEWS.rst index 70db8733..23215d55 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 `_) -- 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 `_) -- 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 `_) +- 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 `_) +- 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 `_) 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 `_) +- Issue number from file names will be stripped down to avoid issue links such as ``#007``. (`#126 `_) - Allow definition of the project ``version`` and ``name`` in the configuration file. This allows use of towncrier seamlessly with non-Python projects. (`#165 `_) - Improve news fragment file name parsing to allow using file names like diff --git a/README.rst b/README.rst index da2250a8..f6cda3e3 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 `_! diff --git a/docs/customization/newsfile.rst b/docs/customization/newsfile.rst index 67f3df4f..03b97a3a 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 258a1323..70b9c410 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 3a4591de..751d4b14 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 77433fca..39fddec0 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 00000000..4cccc128 --- /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. diff --git a/src/towncrier/test/test_build.py b/src/towncrier/test/test_build.py index 23cfb860..85a21bd7 100644 --- a/src/towncrier/test/test_build.py +++ b/src/towncrier/test/test_build.py @@ -914,7 +914,7 @@ def test_bullet_points_false(self, runner): """ 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 9608e0a2..1213e1a3 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 @@ def test_ignores_extension(self): ("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 @@ def test_dots_in_ticket_name_and_counter(self): 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 @@ 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. + when parsing newsfragment names into issue numbers etc. """ self.assertEqual( parse_newfragment_basename(" 007.feature.3", ["feature"]), @@ -132,3 +134,73 @@ def test_orphan_all_digits(self): 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 +""" + )