From f06e52b667658f6884661fa77ca5a4c2a8c1e0e0 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Wed, 6 May 2026 23:30:35 +0000 Subject: [PATCH 1/5] Allow dots in changelog fragment slugs Loosen tools/changelog/cli.py to accept any character that's valid in a git refname segment, except '/' (which would create a subdirectory). The previous rule rejected dots inside the slug, which broke the documented 'use your branch name as the slug' workflow whenever the branch carried a version number (e.g. 'jichuanh/newton-1.2.0rc2-bump' yielded a slug the CLI rejected). Replace the FRAGMENT_RE/SKIP_RE regex pair with a FragmentFilename value object that reverse-parses against a closed suffix tuple (.minor.rst, .major.rst, .skip, .rst). Longest matching suffix wins, so 'foo.minor.rst' is unambiguously slug='foo' tier='minor' regardless of how many dots the slug carries. Slug validation mirrors git check-ref-format: no leading '.' or '-', no trailing '.' or '.lock', no '..' or '@{', no whitespace or '~ ^ : ? * [ \\'. Fragment, FragmentBatch, and PRDiff now share one parsing entry point through FragmentFilename instead of three (cached_property, @staticmethod, and direct regex.match calls). Tests updated to cover dotted version-bearing slugs and the new git-refname rejection cases; fixtures replaced accordingly. --- tools/changelog/cli.py | 140 ++++++++++++++---- .../test/invalid_filenames/-leading-dash.rst | 4 + .../test/invalid_filenames/1234.notabump.rst | 4 - .../has..consecutive-dots.rst | 4 + .../test/invalid_filenames/multi.dot.slug.rst | 4 - tools/changelog/test/test_bump_suffix.py | 33 ++++- tools/changelog/test/test_parse.py | 4 +- tools/changelog/test/test_validate.py | 8 +- 8 files changed, 150 insertions(+), 51 deletions(-) create mode 100644 tools/changelog/test/invalid_filenames/-leading-dash.rst delete mode 100644 tools/changelog/test/invalid_filenames/1234.notabump.rst create mode 100644 tools/changelog/test/invalid_filenames/has..consecutive-dots.rst delete mode 100644 tools/changelog/test/invalid_filenames/multi.dot.slug.rst diff --git a/tools/changelog/cli.py b/tools/changelog/cli.py index 16ad551b4ca3..585c744b0cb4 100644 --- a/tools/changelog/cli.py +++ b/tools/changelog/cli.py @@ -72,15 +72,88 @@ REPO_ROOT = Path(__file__).parent.parent.parent PACKAGES_ROOT = REPO_ROOT / "source" -# Recognised fragment filename patterns. ```` is any short identifier -# the contributor chose — typically their branch name with ``/`` replaced by -# ``-``. The slug must not contain ``.`` (reserved for the tier suffix) or -# ``/`` (path separator), but otherwise mirrors what git allows in a ref name. -# These regexes live at module level because Fragment, FragmentBatch, and -# PRDiff all match against them — they are the wire-format contract between -# contributors and the gate. -FRAGMENT_RE = re.compile(r"^(?P[^./][^./]*)(?:\.(?Pminor|major))?\.rst$") -SKIP_RE = re.compile(r"^(?P[^./][^./]*)\.skip$") + +@dataclass(frozen=True) +class FragmentFilename: + """A fragment's filename parsed into ``(slug, tier)``. + + Wire-format contract between contributors and the gate. Three classes + interpret a filename — :class:`Fragment` (instance, has the file on disk), + :class:`FragmentBatch` (directory walk, filters out skips), and + :class:`PRDiff` (gate, may see paths that don't exist on disk yet) — and + they all need to agree on what counts as a fragment, what tier it + declares, and what slug it owns. Centralising that logic on a value object + keeps the three in lockstep without forcing every caller to materialise a + :class:`Fragment`. + + Suffix matching is anchored from the right with the longest suffix winning + (``foo.minor.rst`` is ``foo`` + minor, never ``foo.minor`` + patch). Slug + rules mirror ``git check-ref-format`` per-segment rules excluding ``/`` — + so dots inside the slug are fine (``bump-newton-1.2.0rc2.minor.rst`` is + valid), which matters because branch names routinely carry version numbers. + """ + + # Recognised filename suffixes, longest first. Exposed as a class + # attribute so tests and the contributor-facing error message can refer to + # the canonical list without re-stating it. + SUFFIXES: ClassVar[tuple[tuple[str, str], ...]] = ( + (".minor.rst", "minor"), + (".major.rst", "major"), + (".skip", "skip"), + (".rst", "patch"), + ) + + # Chars forbidden inside a slug, mirroring ``git check-ref-format``. + _FORBIDDEN_CHARS: ClassVar[frozenset[str]] = frozenset(" ~^:?*[\\\x7f") + + name: str + + @cached_property + def _parsed(self) -> tuple[str, str] | None: + for suffix, tier in self.SUFFIXES: + if not self.name.endswith(suffix): + continue + slug = self.name[: -len(suffix)] + if not self._slug_is_valid(slug): + return None + return slug, tier + return None + + @classmethod + def _slug_is_valid(cls, slug: str) -> bool: + """``True`` if ``slug`` satisfies the git-refname-minus-``/`` rules.""" + if not slug: + return False + if slug[0] in "-." or slug[-1] == ".": + return False + if slug.endswith(".lock") or ".." in slug or "@{" in slug: + return False + return not any(c in cls._FORBIDDEN_CHARS or ord(c) < 32 or c == "/" for c in slug) + + @property + def is_valid(self) -> bool: + """``True`` if the filename parses as either a fragment or a skip marker.""" + return self._parsed is not None + + @property + def is_fragment(self) -> bool: + """``True`` if the filename declares an ``.rst`` fragment (not a skip).""" + return self._parsed is not None and self._parsed[1] != "skip" + + @property + def is_skip(self) -> bool: + """``True`` if the filename is a ``.skip`` marker.""" + return self._parsed is not None and self._parsed[1] == "skip" + + @property + def slug(self) -> str | None: + """Slug component, or ``None`` if the filename does not parse.""" + return self._parsed[0] if self._parsed is not None else None + + @property + def tier(self) -> str | None: + """Bump tier (``patch`` / ``minor`` / ``major`` / ``skip``), or ``None``.""" + return self._parsed[1] if self._parsed is not None else None def _display_path(p: Path) -> str: @@ -154,7 +227,8 @@ class Fragment: A :class:`Fragment` instance is just a path plus methods that interpret it as a changelog fragment. ``.gitkeep`` and ``*.skip`` files should - not be wrapped — only files matching :data:`FRAGMENT_RE`. + not be wrapped — only files whose :class:`FragmentFilename` is + ``is_fragment`` (i.e. an ``.rst`` fragment, not a skip marker). """ path: Path @@ -164,19 +238,22 @@ def name(self) -> str: return self.path.name @cached_property - def _match(self) -> re.Match[str] | None: - return FRAGMENT_RE.match(self.name) + def _filename(self) -> FragmentFilename: + """Cached parsed view of ``self.path.name``.""" + return FragmentFilename(self.name) @property def is_valid_filename(self) -> bool: - return self._match is not None + # ``.skip`` markers parse as :class:`FragmentFilename` but never reach + # :class:`Fragment` — :meth:`FragmentBatch.from_dir` peels them off + # into ``FragmentBatch.skips`` first. Only ``.rst`` fragments need + # content validation and tier aggregation. + return self._filename.is_fragment @property def bump(self) -> str: """Bump tier declared by the filename suffix (defaults to ``'patch'``).""" - if self._match and self._match.group("bump"): - return self._match.group("bump") - return "patch" + return self._filename.tier or "patch" def parse(self) -> dict[str, list[str]]: """Return ``{section: [lines]}`` from this fragment's content. @@ -235,8 +312,7 @@ def parse_slug(filename: str) -> str | None: without needing to materialise a :class:`Fragment` (the diff entry may not exist on disk yet during a gate run). """ - m = FRAGMENT_RE.match(filename) or SKIP_RE.match(filename) - return m.group("slug") if m else None + return FragmentFilename(filename).slug def merge_time(self) -> int: """Unix timestamp of the merge commit that introduced this fragment. @@ -264,17 +340,21 @@ def merge_time(self) -> int: def validate(self) -> str | None: """Return a human-readable error string if malformed, else ``None``. - Filename rules: must match :data:`FRAGMENT_RE` (``.gitkeep`` and - ``*.skip`` files are filtered out at :meth:`FragmentBatch.from_dir` - level and never reach this method). Content rules (for ``*.rst`` - fragments only): non-empty file with at least one valid section - heading and at least one bullet point. + Filename rules: must parse as :class:`FragmentFilename` with + ``is_fragment`` true (``.gitkeep`` and ``*.skip`` files are filtered + out at :meth:`FragmentBatch.from_dir` level and never reach this + method). Content rules (for ``*.rst`` fragments only): non-empty + file with at least one valid section heading and at least one + bullet point. """ if not self.is_valid_filename: return ( "invalid filename — must be .rst, .minor.rst, " - ".major.rst, or .skip (slug = your branch name " - "with `/` replaced by `-`, no dots)" + ".major.rst, or .skip. Slug rules mirror git " + "refnames (excluding `/`): non-empty, no whitespace or any of " + "`~ ^ : ? * [ \\`, no leading `.` or `-`, no trailing `.` or " + "`.lock`, no `..` or `@{`. Dots inside the slug are fine " + "(e.g. `bump-newton-1.2.0rc2.minor.rst`)." ) if not self.path.exists(): # Deleted fragments don't need validating (consumed by a previous compile). @@ -344,7 +424,7 @@ def from_dir(cls, fragment_dir: Path) -> FragmentBatch: for p in fragment_dir.iterdir(): if p.is_dir() or p.name == ".gitkeep": continue - if SKIP_RE.match(p.name): + if FragmentFilename(p.name).is_skip: skips.append(p) continue f = Fragment(p) @@ -761,7 +841,7 @@ def evaluate( continue # Rule 2: content validity (only for *.rst, not *.skip). - if not SKIP_RE.match(path.name): + if not FragmentFilename(path.name).is_skip: err = Fragment(REPO_ROOT / f).validate() if err: invalid_fragments.append((f, err)) @@ -800,11 +880,7 @@ def evaluate( continue # Rule 4: this PR must add at least one valid fragment for the package. - owned = [ - f - for f in fragment_changes - if f in self.added and (FRAGMENT_RE.match(Path(f).name) or SKIP_RE.match(Path(f).name)) - ] + owned = [f for f in fragment_changes if f in self.added and FragmentFilename(Path(f).name).is_valid] if not owned: missing.append(pkg.name) diff --git a/tools/changelog/test/invalid_filenames/-leading-dash.rst b/tools/changelog/test/invalid_filenames/-leading-dash.rst new file mode 100644 index 000000000000..3a24f275fcb9 --- /dev/null +++ b/tools/changelog/test/invalid_filenames/-leading-dash.rst @@ -0,0 +1,4 @@ +Added +^^^^^ + +* Filename has leading dash, which the gate rejects per git-refname rules. diff --git a/tools/changelog/test/invalid_filenames/1234.notabump.rst b/tools/changelog/test/invalid_filenames/1234.notabump.rst deleted file mode 100644 index f70a86344dc6..000000000000 --- a/tools/changelog/test/invalid_filenames/1234.notabump.rst +++ /dev/null @@ -1,4 +0,0 @@ -Added -^^^^^ - -* This file has an unrecognised bump tier and should be rejected. diff --git a/tools/changelog/test/invalid_filenames/has..consecutive-dots.rst b/tools/changelog/test/invalid_filenames/has..consecutive-dots.rst new file mode 100644 index 000000000000..c0a01da597de --- /dev/null +++ b/tools/changelog/test/invalid_filenames/has..consecutive-dots.rst @@ -0,0 +1,4 @@ +Added +^^^^^ + +* Slug contains ``..`` which the gate rejects per git-refname rules. diff --git a/tools/changelog/test/invalid_filenames/multi.dot.slug.rst b/tools/changelog/test/invalid_filenames/multi.dot.slug.rst deleted file mode 100644 index c9ea00434158..000000000000 --- a/tools/changelog/test/invalid_filenames/multi.dot.slug.rst +++ /dev/null @@ -1,4 +0,0 @@ -Added -^^^^^ - -* This file's slug contains dots (reserved for the tier suffix) and should be rejected. diff --git a/tools/changelog/test/test_bump_suffix.py b/tools/changelog/test/test_bump_suffix.py index 1c191c6b33d9..248f1cb3c4bd 100644 --- a/tools/changelog/test/test_bump_suffix.py +++ b/tools/changelog/test/test_bump_suffix.py @@ -94,6 +94,7 @@ def test_aggregate_bump_logic(bumps, expected): @pytest.mark.parametrize( "name,is_fragment,is_skip", [ + # Plain slugs and the three tier suffixes. ("1234.rst", True, False), ("1234.minor.rst", True, False), ("1234.major.rst", True, False), @@ -102,17 +103,39 @@ def test_aggregate_bump_logic(bumps, expected): ("jdoe-add-feature.minor.rst", True, False), ("jdoe-rename-api.major.rst", True, False), ("jdoe-ci-only.skip", False, True), + # Dotted slugs (version-bearing branch names) — accepted; the longest + # matching tier suffix wins, so the slug keeps its embedded dots. + ("bump-newton-1.2.0rc2.minor.rst", True, False), + ("foo.bar.rst", True, False), # slug = ``foo.bar``, tier = patch + ("1234.patch.rst", True, False), # slug = ``1234.patch``, tier = patch + # Files that are not fragments at all. (".gitkeep", False, False), ("README.md", False, False), - ("1234.patch.rst", False, False), # only minor/major are recognised tiers - ("foo.bar.rst", False, False), # extra dots in slug are reserved for tier suffix ("1234.minor", False, False), # missing .rst extension ("1234.rst.bak", False, False), + # Slugs that violate git-refname-style rules: leading ``.`` / ``-``, + # consecutive dots, ``.lock`` ending, forbidden chars, ``/``. + (".leading-dot.rst", False, False), + ("-leading-dash.rst", False, False), + ("trailing-dot..rst", False, False), # `..` not allowed + ("ends-in.lock.rst", False, False), + ("has space.rst", False, False), + ("has~tilde.rst", False, False), + ("has^caret.rst", False, False), + ("nested/path.rst", False, False), ], ) -def test_fragment_filename_regexes(name, is_fragment, is_skip): - assert bool(cli.FRAGMENT_RE.match(name)) is is_fragment - assert bool(cli.SKIP_RE.match(name)) is is_skip +def test_fragment_filename_classifies(name, is_fragment, is_skip): + fn = cli.FragmentFilename(name) + assert fn.is_fragment is is_fragment + assert fn.is_skip is is_skip + + +def test_fragment_filename_extracts_dotted_slug_and_tier(): + """Slugs with dots round-trip when paired with a tier suffix.""" + fn = cli.FragmentFilename("bump-newton-1.2.0rc2.minor.rst") + assert fn.slug == "bump-newton-1.2.0rc2" + assert fn.tier == "minor" # --------------------------------------------------------------------------- diff --git a/tools/changelog/test/test_parse.py b/tools/changelog/test/test_parse.py index fe8123c2f14c..37682404d0d8 100644 --- a/tools/changelog/test/test_parse.py +++ b/tools/changelog/test/test_parse.py @@ -66,10 +66,10 @@ def test_parse_fragment_no_section_headings(tmp_path): def test_fragment_batch_flags_invalid_filenames_from_fixture(): - """Files with dotted slugs or unknown bump tiers go in ``invalid``.""" + """Files violating git-refname slug rules go in ``invalid``.""" batch = cli.FragmentBatch.from_dir(FIXTURES / "invalid_filenames") assert batch.valid == [] - assert {p.name for p in batch.invalid} == {"multi.dot.slug.rst", "1234.notabump.rst"} + assert {p.name for p in batch.invalid} == {"-leading-dash.rst", "has..consecutive-dots.rst"} def test_fragment_batch_missing_directory(tmp_path): diff --git a/tools/changelog/test/test_validate.py b/tools/changelog/test/test_validate.py index e02e1b95e990..9255d4b96a61 100644 --- a/tools/changelog/test/test_validate.py +++ b/tools/changelog/test/test_validate.py @@ -45,13 +45,13 @@ def test_validate_accepts_major_suffix(tmp_path): # --------------------------------------------------------------------------- -def test_validate_rejects_unknown_filename_from_fixture(): - err = cli.Fragment(FIXTURES / "invalid_filenames" / "multi.dot.slug.rst").validate() +def test_validate_rejects_leading_dash_slug_from_fixture(): + err = cli.Fragment(FIXTURES / "invalid_filenames" / "-leading-dash.rst").validate() assert err is not None and "invalid filename" in err -def test_validate_rejects_unknown_bump_tier_from_fixture(): - err = cli.Fragment(FIXTURES / "invalid_filenames" / "1234.notabump.rst").validate() +def test_validate_rejects_consecutive_dots_slug_from_fixture(): + err = cli.Fragment(FIXTURES / "invalid_filenames" / "has..consecutive-dots.rst").validate() assert err is not None and "invalid filename" in err From 8263fc3fbf0dc0b6f2dcf3a3ba3cbdc04987831a Mon Sep 17 00:00:00 2001 From: jichuanh Date: Wed, 6 May 2026 23:32:59 +0000 Subject: [PATCH 2/5] Add unit tests covering every FragmentFilename branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fill the remaining coverage gaps so every public property and every private branch of the new value object is hit by a focused test: - test_fragment_filename_slug_and_tier: one case per tier (patch / minor / major / skip), plus the dotted-slug and 'no match' outcomes — covers .slug and .tier for both populated and None paths. - test_fragment_filename_validity_and_kind: 4-row grid asserting the .is_valid / .is_fragment / .is_skip flags stay consistent with each other across fragments, skips, and unparseable names. - test_fragment_filename_rejects_forbidden_chars: parametrized over every character in FragmentFilename._FORBIDDEN_CHARS plus a representative ASCII control char and DEL, so each character is covered individually rather than relying on collective coverage from a few example slugs. - test_fragment_filename_rejects_structural_edge_cases: empty filename, suffix-only filenames (slug would be empty), and the '@{' substring git refnames forbid. - test_fragment_filename_suffixes_are_canonical: pins the SUFFIXES class attribute to the exact tuple it must remain — the wire format for fragment filenames is part of the contract. --- tools/changelog/test/test_bump_suffix.py | 84 ++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/tools/changelog/test/test_bump_suffix.py b/tools/changelog/test/test_bump_suffix.py index 248f1cb3c4bd..cd3ebfc47056 100644 --- a/tools/changelog/test/test_bump_suffix.py +++ b/tools/changelog/test/test_bump_suffix.py @@ -138,6 +138,90 @@ def test_fragment_filename_extracts_dotted_slug_and_tier(): assert fn.tier == "minor" +@pytest.mark.parametrize( + "name,expected_slug,expected_tier", + [ + # One representative case per tier so each branch of the SUFFIXES + # tuple is exercised on its own. + ("plain.rst", "plain", "patch"), + ("with-feature.minor.rst", "with-feature", "minor"), + ("with-break.major.rst", "with-break", "major"), + ("ci-only.skip", "ci-only", "skip"), + # Dotted slug carries the most-specific suffix. + ("v1.2.3-bump.major.rst", "v1.2.3-bump", "major"), + # Filenames that don't match any suffix yield ``(None, None)``. + ("not-a-fragment", None, None), + ("README.md", None, None), + ], +) +def test_fragment_filename_slug_and_tier(name, expected_slug, expected_tier): + fn = cli.FragmentFilename(name) + assert fn.slug == expected_slug + assert fn.tier == expected_tier + + +@pytest.mark.parametrize( + "name,is_valid,is_fragment,is_skip", + [ + # ``is_valid`` is true for both fragments and skip markers; only the + # latter two flags partition the parsed names. This grid asserts + # they're consistent for the four interesting outcomes. + ("plain.rst", True, True, False), + ("plain.minor.rst", True, True, False), + ("ci-only.skip", True, False, True), + ("not-a-fragment", False, False, False), + ], +) +def test_fragment_filename_validity_and_kind(name, is_valid, is_fragment, is_skip): + fn = cli.FragmentFilename(name) + assert fn.is_valid is is_valid + assert fn.is_fragment is is_fragment + assert fn.is_skip is is_skip + + +@pytest.mark.parametrize( + "bad_char", + # Each forbidden char in :attr:`FragmentFilename._FORBIDDEN_CHARS` plus a + # representative ASCII control char and the ``DEL`` sentinel — the regex + # used to call these out via membership checks; the parser should still + # reject them per character. + [" ", "~", "^", ":", "?", "*", "[", "\\", "\x01", "\x7f"], +) +def test_fragment_filename_rejects_forbidden_chars(bad_char): + fn = cli.FragmentFilename(f"slug{bad_char}with-bad-char.rst") + assert fn.is_valid is False + assert fn.slug is None + assert fn.tier is None + + +@pytest.mark.parametrize( + "name", + # Edge cases that don't fit cleanly into the parametrized validity grid: + # an empty filename, a filename that's *only* a suffix (slug would be + # empty), and the ``@{`` substring git refnames forbid. + [ + "", + ".rst", + ".minor.rst", + ".skip", + "has@{atbrace}.rst", + ], +) +def test_fragment_filename_rejects_structural_edge_cases(name): + fn = cli.FragmentFilename(name) + assert fn.is_valid is False + + +def test_fragment_filename_suffixes_are_canonical(): + """``SUFFIXES`` is the wire-format contract — pin the exact tuple.""" + assert cli.FragmentFilename.SUFFIXES == ( + (".minor.rst", "minor"), + (".major.rst", "major"), + (".skip", "skip"), + (".rst", "patch"), + ) + + # --------------------------------------------------------------------------- # Fragment.parse_slug — derived from filename for collision detection # --------------------------------------------------------------------------- From 8a8ebf8eff13d50f579286450d7b1fbec41b7758 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Wed, 6 May 2026 23:44:39 +0000 Subject: [PATCH 3/5] Address greptile review on PR #5525 - Drop the dead 'slug is None' guard in PRDiff.check_fragments. Under the new FragmentFilename design, every file reaching Rule 3 has either passed is_skip filtering (which only accepts a successfully parsed filename) or passed Fragment.validate (which rejects unparseable names), so parse_slug always returns a valid slug. Replace the dead branch with an assert documenting the invariant for future readers. - Fix a misleading test comment: 'trailing-dot..rst' is rejected by the trailing-'.' rule (the slug after stripping '.rst' is 'trailing-dot.'), not by the '..' rule. - Add 'has..consecutive.rst' as a separate parametrized case so the '..' rejection rule is exercised by its own filename in test_fragment_filename_classifies. --- tools/changelog/cli.py | 12 +++++------- tools/changelog/test/test_bump_suffix.py | 3 ++- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/tools/changelog/cli.py b/tools/changelog/cli.py index 585c744b0cb4..fa7f75456436 100644 --- a/tools/changelog/cli.py +++ b/tools/changelog/cli.py @@ -848,14 +848,12 @@ def evaluate( continue # Rule 3: slug uniqueness within the package's changelog.d/. + # By construction ``parse_slug`` returns a valid slug here: + # ``.skip`` files passed the ``is_skip`` filter (which only + # accepts a successfully parsed filename), and ``.rst`` files + # passed ``validate()`` (which rejects unparseable names). slug = Fragment.parse_slug(path.name) - if slug is None: - # Filename validation already flagged this above for *.rst, - # but a malformed *.skip would slip through. Surface it. - invalid_fragments.append( - (f, "invalid filename — must be .rst, .minor.rst, .major.rst, or .skip") - ) - continue + assert slug is not None, f"unreachable: {path.name!r} reached Rule 3 without a valid slug" if slug in existing_slugs and existing_slugs[slug] != path.name: invalid_fragments.append( ( diff --git a/tools/changelog/test/test_bump_suffix.py b/tools/changelog/test/test_bump_suffix.py index cd3ebfc47056..eed613c765a4 100644 --- a/tools/changelog/test/test_bump_suffix.py +++ b/tools/changelog/test/test_bump_suffix.py @@ -117,7 +117,8 @@ def test_aggregate_bump_logic(bumps, expected): # consecutive dots, ``.lock`` ending, forbidden chars, ``/``. (".leading-dot.rst", False, False), ("-leading-dash.rst", False, False), - ("trailing-dot..rst", False, False), # `..` not allowed + ("trailing-dot..rst", False, False), # slug ``trailing-dot.`` ends in `.` + ("has..consecutive.rst", False, False), # slug contains `..` ("ends-in.lock.rst", False, False), ("has space.rst", False, False), ("has~tilde.rst", False, False), From cae2a108fd6fab33cec853e12d23a631e3005fb1 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Wed, 6 May 2026 23:52:37 +0000 Subject: [PATCH 4/5] Address codex review on PR #5525 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three actionable items from the review: 1. Centralise the user-facing suffix list. Three places (Fragment.validate error, compile() warning, missing-fragment help block) had the suffix list hardcoded as a string literal, so adding a new tier to FragmentFilename.SUFFIXES would silently leave the help text out of sync. Replace all three with FragmentFilename.pattern_summary() and FragmentFilename.help_lines_for_package(), both derived from SUFFIXES. Output is byte-for-byte identical to the previous strings, including the column alignment in the help block. 2. Document the foo.skip.rst footgun. A slug ending in '.skip' paired with '.rst' parses as a *patch* fragment with slug 'foo.skip', not as a skip marker — '.skip' is its own suffix, mutually exclusive with '.rst'. Note this in Fragment.validate's error message so contributors who hit it understand the parse without reading the source. 3. Lock in foo.skip.rst behavior with a parametrized test case. Also pin the new pattern_summary() and help_lines_for_package() helpers against the exact strings they currently produce, so any future SUFFIXES change has to consciously update these too. --- tools/changelog/cli.py | 69 ++++++++++++++++++++---- tools/changelog/test/test_bump_suffix.py | 22 ++++++++ 2 files changed, 80 insertions(+), 11 deletions(-) diff --git a/tools/changelog/cli.py b/tools/changelog/cli.py index fa7f75456436..cf7c537e5b0a 100644 --- a/tools/changelog/cli.py +++ b/tools/changelog/cli.py @@ -155,6 +155,53 @@ def tier(self) -> str | None: """Bump tier (``patch`` / ``minor`` / ``major`` / ``skip``), or ``None``.""" return self._parsed[1] if self._parsed is not None else None + # ---- User-facing pattern descriptions (derived from SUFFIXES) --------- + + # Display order for help / error messages. The parser order in + # :attr:`SUFFIXES` is "longest suffix first" (semantically required), but + # readers prefer "tiers ascending" (patch → minor → major → skip). + _DISPLAY_ORDER: ClassVar[tuple[str, ...]] = ("patch", "minor", "major", "skip") + + @classmethod + def pattern_summary(cls) -> str: + """Comma-separated list of accepted patterns: ``.rst, ..., or .skip``. + + Single source of truth for the user-facing pattern list. Derived from + :attr:`SUFFIXES` so that adding a tier updates every error message + and help block at once. + """ + by_tier = {tier: suffix for suffix, tier in cls.SUFFIXES} + parts = [f"{by_tier[t]}" for t in cls._DISPLAY_ORDER if t in by_tier] + return ", ".join(parts[:-1]) + f", or {parts[-1]}" + + @classmethod + def help_lines_for_package(cls, package_name: str) -> list[str]: + """Per-tier help lines used when a package is missing a fragment. + + Returns one ``add ...`` / ``or ...`` line per tier, formatted with + the path under the package's ``changelog.d/`` directory and an inline + annotation describing the bump. + """ + annotations = { + "patch": "(patch bump)", + "minor": "(minor bump)", + "major": "(major bump)", + "skip": "(no entry, no bump)", + } + by_tier = {tier: suffix for suffix, tier in cls.SUFFIXES} + # Pad the suffix column so the annotations line up regardless of tier + # length — purely cosmetic, but the existing CI output already aligns. + suffix_width = max(len(s) for s in by_tier.values()) + lines: list[str] = [] + for i, t in enumerate(cls._DISPLAY_ORDER): + if t not in by_tier: + continue + verb = "add " if i == 0 else "or " + path = f"source/{package_name}/changelog.d/{by_tier[t]}" + padding = " " * (suffix_width - len(by_tier[t])) + lines.append(f"{verb} {path}{padding} {annotations[t]}") + return lines + def _display_path(p: Path) -> str: """Pretty-print a Path. Strips ``REPO_ROOT`` if ``p`` is inside the repo, @@ -349,12 +396,14 @@ def validate(self) -> str | None: """ if not self.is_valid_filename: return ( - "invalid filename — must be .rst, .minor.rst, " - ".major.rst, or .skip. Slug rules mirror git " - "refnames (excluding `/`): non-empty, no whitespace or any of " - "`~ ^ : ? * [ \\`, no leading `.` or `-`, no trailing `.` or " - "`.lock`, no `..` or `@{`. Dots inside the slug are fine " - "(e.g. `bump-newton-1.2.0rc2.minor.rst`)." + f"invalid filename — must be {FragmentFilename.pattern_summary()}. " + "Slug rules mirror git refnames (excluding `/`): non-empty, no " + "whitespace or any of `~ ^ : ? * [ \\`, no leading `.` or `-`, " + "no trailing `.` or `.lock`, no `..` or `@{`. Dots inside the " + "slug are fine (e.g. `bump-newton-1.2.0rc2.minor.rst`). " + "Note: a slug ending in `.skip` paired with `.rst` (e.g. " + "`foo.skip.rst`) parses as a *patch fragment* with slug " + "`foo.skip`, not as a skip marker — `.skip` is its own suffix." ) if not self.path.exists(): # Deleted fragments don't need validating (consumed by a previous compile). @@ -663,7 +712,7 @@ def compile( for p in batch.invalid: print( f" WARNING: {_display_path(p)} does not match any recognised fragment " - "pattern (.rst, .minor.rst, .major.rst, .skip) — skipping.", + f"pattern ({FragmentFilename.pattern_summary()}) — skipping.", file=sys.stderr, ) @@ -959,10 +1008,8 @@ def cmd_check(args: argparse.Namespace, _parser: argparse.ArgumentParser) -> int print("::error::Missing changelog fragments for the following packages:") for pkg_name in missing: print(f" • {pkg_name}") - print(f" → add source/{pkg_name}/changelog.d/.rst (patch bump)") - print(f" → or source/{pkg_name}/changelog.d/.minor.rst (minor bump)") - print(f" → or source/{pkg_name}/changelog.d/.major.rst (major bump)") - print(f" → or source/{pkg_name}/changelog.d/.skip (no entry, no bump)") + for line in FragmentFilename.help_lines_for_package(pkg_name): + print(f" → {line}") print() print("Slug = your branch name with `/` replaced by `-` (or any short, unique name).") print() diff --git a/tools/changelog/test/test_bump_suffix.py b/tools/changelog/test/test_bump_suffix.py index eed613c765a4..bea1cb98e446 100644 --- a/tools/changelog/test/test_bump_suffix.py +++ b/tools/changelog/test/test_bump_suffix.py @@ -108,6 +108,12 @@ def test_aggregate_bump_logic(bumps, expected): ("bump-newton-1.2.0rc2.minor.rst", True, False), ("foo.bar.rst", True, False), # slug = ``foo.bar``, tier = patch ("1234.patch.rst", True, False), # slug = ``1234.patch``, tier = patch + # Pin the easy contributor footgun: ``foo.skip.rst`` is a *patch + # fragment* with slug ``foo.skip`` (the file extension is ``.rst``), + # not a skip marker — ``.skip`` is its own suffix, mutually + # exclusive with ``.rst``. Locking this in so a future "fix" can't + # silently flip the semantics. + ("foo.skip.rst", True, False), # Files that are not fragments at all. (".gitkeep", False, False), ("README.md", False, False), @@ -223,6 +229,22 @@ def test_fragment_filename_suffixes_are_canonical(): ) +def test_fragment_filename_pattern_summary_is_derived_from_suffixes(): + """User-facing list keeps tiers in display order and ends with ``or``.""" + assert cli.FragmentFilename.pattern_summary() == (".rst, .minor.rst, .major.rst, or .skip") + + +def test_fragment_filename_help_lines_format_per_tier(): + """Help lines for a missing package fragment cover every tier with aligned columns.""" + lines = cli.FragmentFilename.help_lines_for_package("isaaclab_newton") + assert lines == [ + "add source/isaaclab_newton/changelog.d/.rst (patch bump)", + "or source/isaaclab_newton/changelog.d/.minor.rst (minor bump)", + "or source/isaaclab_newton/changelog.d/.major.rst (major bump)", + "or source/isaaclab_newton/changelog.d/.skip (no entry, no bump)", + ] + + # --------------------------------------------------------------------------- # Fragment.parse_slug — derived from filename for collision detection # --------------------------------------------------------------------------- From ba74fb6db58dcb53e631c875c729281b9d653842 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Fri, 15 May 2026 22:50:52 +0000 Subject: [PATCH 5/5] Fix codespell typo: unparseable -> unparsable --- tools/changelog/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/changelog/cli.py b/tools/changelog/cli.py index 996cb94efa53..cc4c865713e3 100644 --- a/tools/changelog/cli.py +++ b/tools/changelog/cli.py @@ -922,7 +922,7 @@ def evaluate( # By construction ``parse_slug`` returns a valid slug here: # ``.skip`` files passed the ``is_skip`` filter (which only # accepts a successfully parsed filename), and ``.rst`` files - # passed ``validate()`` (which rejects unparseable names). + # passed ``validate()`` (which rejects unparsable names). slug = Fragment.parse_slug(path.name) assert slug is not None, f"unreachable: {path.name!r} reached Rule 3 without a valid slug" if slug in existing_slugs and existing_slugs[slug] != path.name: