[changelog] Allow dots in fragment slugs#5525
Conversation
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.
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.
Greptile SummaryThis PR replaces two module-level regexes (
Confidence Score: 4/5Safe to merge — the change is confined to CI tooling and does not touch any package source; the suffix-priority algorithm and slug validation are correct, and the 92-test suite covers the new acceptance and rejection cases thoroughly. The refactor is well-structured and the new FragmentFilename value object correctly handles all the edge cases tested. Two small cleanup opportunities exist: a stale comment inside PRDiff.check_fragments that describes a now-unreachable code path, and a misleading inline comment in a parametrized test case. Neither affects runtime behaviour. The dead-code comment in tools/changelog/cli.py around line 852 is worth a quick read to confirm the surrounding guard is understood before future edits to that gate logic. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # consecutive dots, ``.lock`` ending, forbidden chars, ``/``. | ||
| (".leading-dot.rst", False, False), | ||
| ("-leading-dash.rst", False, False), | ||
| ("trailing-dot..rst", False, False), # `..` not allowed |
There was a problem hiding this comment.
.rst suffix the slug is trailing-dot., which is rejected by the trailing-dot rule (slug[-1] == ".") — not because .. appears inside the slug.
| ("trailing-dot..rst", False, False), # `..` not allowed | |
| ("trailing-dot..rst", False, False), # slug = ``trailing-dot.`` — rejected for trailing ``.`` |
- 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.
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.
…-allow-dots-in-slug
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: [changelog] Allow dots in fragment slugs
Summary
This PR addresses a real pain point: the changelog fragment gate was rejecting branch-name-derived slugs containing dots (like version numbers bump-newton-1.2.0rc2), which contradicted the documented workflow in AGENTS.md. The solution replaces brittle regex patterns with a well-designed FragmentFilename value object that mirrors git refname validation rules.
Verdict: ✅ Approve — This is a well-crafted refactoring with thorough test coverage. The changes improve maintainability while fixing the documented issue.
Strengths
1. Excellent OO Consolidation
The shift from scattered FRAGMENT_RE/SKIP_RE regex calls to a centralized FragmentFilename dataclass is a significant improvement. Previously, three different code paths parsed filenames in three different ways — now there's a single source of truth.
2. Robust Validation Logic
The slug validation mirrors git check-ref-format rules, which is the right choice: it's well-documented behavior that contributors already understand. The suffix matching strategy (longest match wins) elegantly handles ambiguous cases like foo.minor.rst vs foo.bar.minor.rst.
3. Comprehensive Test Coverage
The test suite is thorough:
- Parametrized tests covering all tier suffixes
- Edge cases for every forbidden character
- Structural edge cases (empty filenames, suffix-only names,
@{substring) - Pinned tests for
SUFFIXEStuple andpattern_summary()to catch drift - The
foo.skip.rstfootgun is explicitly documented and tested
4. Improved Error Messages
The contributor-facing error now clearly explains the rules — no more digging through source code to understand why a dotted slug was rejected.
Minor Observations
1. Assertion vs Defensive Error (informational)
slug = Fragment.parse_slug(path.name)
assert slug is not None, f"unreachable: {path.name!r} reached Rule 3 without a valid slug"The assertion documents the invariant clearly. This is appropriate since the code path is genuinely unreachable given the preceding validation gates. If this ever fires, it indicates a logic bug that should fail loudly rather than be silently handled.
2. _FORBIDDEN_CHARS Completeness
The forbidden set frozenset(" ~^:?*[\\\x7f") covers git's refname rules. The ord(c) < 32 check catches other control characters. This is correct and complete.
3. Display Order vs Parse Order
The _DISPLAY_ORDER tuple being separate from SUFFIXES is intentional and well-documented — parse order needs longest-first semantics, display order is for humans.
Test Observations
The test fixtures were updated appropriately:
- Removed
multi.dot.slug.rstand1234.notabump.rst(now valid) - Added
-leading-dash.rstandhas..consecutive-dots.rst(actually invalid)
The 92 passing tests mentioned in the PR description appear comprehensive.
CI Status
Pre-commit and changelog fragment checks pass. Other CI jobs are still pending but are unrelated to the tooling changes in this PR.
LGTM 👍
Update (db68e3a): New commits are a merge from develop bringing in unrelated changes (Docker non-root user, camera/renderer warp migration, frame stacking, teleop docs, OVRTX fixes). No changes to the changelog tooling code itself.
✅ Previous inline comment addressed: The comment on trailing-dot..rst was clarified to indicate rejection is due to the trailing . in the slug, not the presence of ...
No new issues introduced. LGTM remains.
1 participant
Summary
The changelog fragment gate (`tools/changelog/cli.py`) currently rejects any slug containing a dot. That contradicts AGENTS.md's recommendation to use the branch name as the slug, since branches routinely carry version numbers — `bump-newton-1.2.0rc2`, `fix-foo-v2.5`, etc. Following the docs literally produces a slug that the CLI then rejects, requiring a manual rename step on every version-bearing PR.
This PR loosens the parser to accept any character valid in a git refname segment, except `/` (which would create a subdirectory inside `changelog.d/`).
What changed
Parser
OO consolidation
Before this PR, three classes parsed filenames three different ways:
After: all three go through one `FragmentFilename` value object that exposes `.is_fragment`, `.is_skip`, `.slug`, `.tier`. One source of truth, no scattered regex matches.
Error message
The contributor-facing error now spells out the rules explicitly so a future contributor with a dotted branch name doesn't have to dig through the source:
```
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`).
```
Tests
Test plan
Why no changelog fragment
This PR only touches `tools/changelog/` (CI tooling), not any package under `source/`. The gate only requires fragments for touched `source/` packages.