feat: add package changelog viewer#73
Open
patrykcodex-del wants to merge 5 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an offline Python CLI ( ChangesChangelog Viewer Feature
Sequence DiagramsequenceDiagram
participant CLI as CLI entry
participant Loader as load_entries
participant Filter as filter_entries
participant Compare as compare_entries
participant Formatter as format_entries
participant Exporter as export_entries
participant IO as stdout/file
CLI->>Loader: parse args, resolve/load changelog
Loader->>Filter: return parsed entries
Filter->>Compare: apply --older/--newer if provided
Compare->>Formatter: select and order entries
Formatter->>IO: print formatted output
opt --export set
Formatter->>Exporter: prepare JSON payload
Exporter->>IO: write JSON file
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Debian package changelog viewer utility (scripts/changelog.py) that allows users to view, search, compare, and export package changelogs. It also adds a corresponding test suite (tests/test_changelog.py), updates the Makefile with a test-changelog target, and documents the tool in the README.md. The code reviewer provided valuable feedback to enhance the utility's robustness, such as improving parsing regexes, making the package argument optional when a file is explicitly provided, automatically creating parent directories on export, and adding validation for version comparison inputs.
| r"^(?P<package>[\w.+-]+) \((?P<version>[^)]+)\) (?P<distribution>[^;]+); urgency=(?P<urgency>\S+)" | ||
| ) | ||
| SECURITY_RE = re.compile(r"\b(CVE-\d{4}-\d{4,}|security|vulnerab|exploit|privilege|auth)\b", re.I) | ||
| BULLET_RE = re.compile(r"^\s{2,}\*\s?(?P<text>.*)$") |
There was a problem hiding this comment.
Debian changelogs conventionally use * for bullet points, but some maintainers or tools use - as well. Updating the regex to support both * and - makes the parser more robust.
Suggested change
| BULLET_RE = re.compile(r"^\s{2,}\*\s?(?P<text>.*)$") | |
| BULLET_RE = re.compile(r"^\s{2,}[*-]\s?(?P<text>.*)$") |
| ) | ||
| SECURITY_RE = re.compile(r"\b(CVE-\d{4}-\d{4,}|security|vulnerab|exploit|privilege|auth)\b", re.I) | ||
| BULLET_RE = re.compile(r"^\s{2,}\*\s?(?P<text>.*)$") | ||
| MAINTAINER_RE = re.compile(r"^ -- (?P<maintainer>.+?) (?P<date>.+)$") |
There was a problem hiding this comment.
Using \s{2,} instead of two literal spaces is more flexible and handles cases where there might be extra spaces before the date, while still preventing the non-greedy .+? from matching spaces within the maintainer's name.
Suggested change
| MAINTAINER_RE = re.compile(r"^ -- (?P<maintainer>.+?) (?P<date>.+)$") | |
| MAINTAINER_RE = re.compile(r"^ -- (?P<maintainer>.+?)\s{2,}(?P<date>.+)$") |
Comment on lines
+96
to
+100
| def load_entries(package: str, changelog: Path | None = None) -> list[ChangelogEntry]: | ||
| path = changelog or default_changelog_path(package) | ||
| if not path.exists(): | ||
| raise FileNotFoundError(f"No changelog found for {package!r}: {path}") | ||
| return parse_changelog(path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
Allow package to be optional when changelog is explicitly provided via --file. This avoids requiring a dummy package name on the CLI when auditing a specific file.
Suggested change
| def load_entries(package: str, changelog: Path | None = None) -> list[ChangelogEntry]: | |
| path = changelog or default_changelog_path(package) | |
| if not path.exists(): | |
| raise FileNotFoundError(f"No changelog found for {package!r}: {path}") | |
| return parse_changelog(path.read_text(encoding="utf-8")) | |
| def load_entries(package: str | None, changelog: Path | None = None) -> list[ChangelogEntry]: | |
| path = changelog or (default_changelog_path(package) if package else None) | |
| if not path: | |
| raise ValueError("Either package or changelog file must be specified") | |
| if not path.exists(): | |
| msg = f"No changelog found for {package!r}: {path}" if package else f"No changelog found: {path}" | |
| raise FileNotFoundError(msg) | |
| return parse_changelog(path.read_text(encoding="utf-8")) |
Comment on lines
+136
to
+142
| def export_entries(entries: Iterable[ChangelogEntry], output: Path) -> None: | ||
| data = [] | ||
| for entry in entries: | ||
| item = asdict(entry) | ||
| item["has_security_fix"] = entry.has_security_fix | ||
| data.append(item) | ||
| output.write_text(json.dumps(data, indent=2) + "\n", encoding="utf-8") |
There was a problem hiding this comment.
If the parent directory of the export path does not exist, write_text will raise a FileNotFoundError. Creating the parent directories automatically improves usability.
Suggested change
| def export_entries(entries: Iterable[ChangelogEntry], output: Path) -> None: | |
| data = [] | |
| for entry in entries: | |
| item = asdict(entry) | |
| item["has_security_fix"] = entry.has_security_fix | |
| data.append(item) | |
| output.write_text(json.dumps(data, indent=2) + "\n", encoding="utf-8") | |
| def export_entries(entries: Iterable[ChangelogEntry], output: Path) -> None: | |
| data = [] | |
| for entry in entries: | |
| item = asdict(entry) | |
| item["has_security_fix"] = entry.has_security_fix | |
| data.append(item) | |
| output.parent.mkdir(parents=True, exist_ok=True) | |
| output.write_text(json.dumps(data, indent=2) + "\n", encoding="utf-8") |
Comment on lines
+145
to
+147
| def build_parser() -> argparse.ArgumentParser: | ||
| parser = argparse.ArgumentParser(description="View/search package changelogs") | ||
| parser.add_argument("package", help="package directory name under packages/") |
There was a problem hiding this comment.
Make the package positional argument optional (nargs='?') so that it is not required when --file is specified.
Suggested change
| def build_parser() -> argparse.ArgumentParser: | |
| parser = argparse.ArgumentParser(description="View/search package changelogs") | |
| parser.add_argument("package", help="package directory name under packages/") | |
| def build_parser() -> argparse.ArgumentParser: | |
| parser = argparse.ArgumentParser(description="View/search package changelogs") | |
| parser.add_argument("package", nargs="?", help="package directory name under packages/") |
Comment on lines
+157
to
+169
| def main(argv: list[str] | None = None) -> int: | ||
| args = build_parser().parse_args(argv) | ||
| try: | ||
| entries = load_entries(args.package, args.file) | ||
| except FileNotFoundError as exc: | ||
| print(exc, file=sys.stderr) | ||
| return 2 | ||
|
|
||
| if args.older or args.newer: | ||
| if not (args.older and args.newer): | ||
| print("compare mode requires both older and newer versions", file=sys.stderr) | ||
| return 2 | ||
| entries = compare_entries(entries, args.older, args.newer) |
There was a problem hiding this comment.
Add validation to ensure that:
- Either
packageor--fileis provided. - Both
olderandnewerversions exist in the changelog when using compare mode. - The
newerversion is actually newer than (or appears before) theolderversion in the changelog, preventing silent empty outputs due to incorrect ordering or typos.
def main(argv: list[str] | None = None) -> int:
parser = build_parser()
args = parser.parse_args(argv)
if not args.package and not args.file:
parser.error("Either package or --file must be specified.")
try:
entries = load_entries(args.package, args.file)
except FileNotFoundError as exc:
print(exc, file=sys.stderr)
return 2
if args.older or args.newer:
if not (args.older and args.newer):
print("compare mode requires both older and newer versions", file=sys.stderr)
return 2
versions = [e.version for e in entries]
if args.older not in versions:
print(f"Error: older version {args.older!r} not found in changelog", file=sys.stderr)
return 2
if args.newer not in versions:
print(f"Error: newer version {args.newer!r} not found in changelog", file=sys.stderr)
return 2
if versions.index(args.newer) > versions.index(args.older):
print(f"Error: version {args.newer!r} is older than {args.older!r} in the changelog", file=sys.stderr)
return 2
entries = compare_entries(entries, args.older, args.newer)![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/changelog.py`:
- Around line 111-121: The compare_entries function currently returns
partial/empty results when the provided newer/older versions are missing or
reversed; update compare_entries to validate that both newer and older exist in
the entries and that newer is encountered before older—if validation fails,
print a clear error message and exit non-zero (or raise SystemExit) instead of
returning a silent result; apply the same validation/failure behavior to the
other compare logic referenced around the 165-170 block so all compare paths
consistently fail fast when bounds are invalid.
- Around line 175-177: Wrap the export_entries(entries, args.export) call in
main with an OSError handler so filesystem/write errors produce a controlled CLI
failure: catch OSError as e, print or log a concise error message including
str(e) to stderr, and exit with a non-zero CLI code (e.g., sys.exit(2)); ensure
sys is imported if not present. This change should live around the
export_entries invocation in main (the args.export branch) so invalid
paths/permissions don't raise an unhandled traceback.
In `@tests/test_changelog.py`:
- Around line 49-75: The test test_export_json_and_cli_output calls
subprocess.run(...) to invoke scripts/changelog.py which still prints formatted
entries after calling export_entries (because scripts/changelog.py calls
format_entries(entries) and print(output)), and Ruff flags S603 on that
subprocess.run; fix by either adding a linter exemption comment "# noqa: S603"
to the subprocess.run(...) invocation in test_export_json_and_cli_output (to
silence the Ruff warning) or, better, remove/guard the post-export print in
scripts/changelog.py (where format_entries(entries) and print(output) are
called) so that when export_entries(...) is used no CLI output is printed;
reference symbols: test_export_json_and_cli_output, subprocess.run,
scripts/changelog.py, export_entries, format_entries, and print(output).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f79d144-c1e6-49c9-92f2-bc2597e4a2cb
📒 Files selected for processing (4)
MakefileREADME.mdscripts/changelog.pytests/test_changelog.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_changelog.py (1)
106-128:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSilence Ruff S603 on the
subprocess.runinvocation.Ruff flags
S603on thissubprocess.run(...)call (error-level), which can break a lint-gated pipeline. The inputs are static/trusted (sys.executable+ repo path + literal flags), so suppress it inline. Apply the same treatment to the analogous call at Line 78 for consistency.🧰 Proposed fix
result = subprocess.run( [ sys.executable, str(ROOT / "scripts" / "changelog.py"), "docker", "24.0.5-1", "24.0.7-1", "--file", str(source), ], check=False, text=True, capture_output=True, - ) + ) # noqa: S603🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_changelog.py` around lines 106 - 128, Add an inline Ruff suppression for S603 on the subprocess.run invocation in test_cli_rejects_missing_or_invalid_compare_bounds by appending a "# noqa: S603" comment to the subprocess.run(...) line to silence the warning for this trusted/static input; also apply the same inline "# noqa: S603" suppression to the analogous subprocess.run call earlier in the same test module (the other test that runs the changelog script) so both calls consistently suppress Ruff S603.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/test_changelog.py`:
- Around line 106-128: Add an inline Ruff suppression for S603 on the
subprocess.run invocation in test_cli_rejects_missing_or_invalid_compare_bounds
by appending a "# noqa: S603" comment to the subprocess.run(...) line to silence
the warning for this trusted/static input; also apply the same inline "# noqa:
S603" suppression to the analogous subprocess.run call earlier in the same test
module (the other test that runs the changelog script) so both calls
consistently suppress Ruff S603.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa1d3110-54af-4264-8c6b-acd7a1a824c9
📒 Files selected for processing (2)
scripts/changelog.pytests/test_changelog.py
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/changelog.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scripts/changelog.pyfor local Debian package changelogs underpackages/<name>/debian/changelogmake test-changelogtarget and README usage examplesTest plan
python3 -m py_compile scripts/changelog.py tests/test_changelog.pypython3 -m unittest tests/test_changelog.pymake test-changelogpython3 scripts/changelog.py cx-core --security --export /tmp/cx-core-security.jsongit diff --checkRefs #35
Summary by CodeRabbit
New Features
Chores
test-changelogand documented it in help/.PHONY.testtarget to run changelog unit tests.Documentation
Tests