fix(cliff): render commit body lists and drop repository artifacts - #31
fix(cliff): render commit body lists and drop repository artifacts#31ThetaSinner wants to merge 6 commits into
Conversation
Generated changelogs had four problems, all visible in released notes: - Multi-line lists in a commit body were flattened onto a single line, because the template joined every newline in a paragraph into a space and could not tell a list item from a hard-wrapped continuation. List items are now marked during preprocessing and rendered as a nested list, with continuation lines joined back onto their item. - The "# Conflicts:" block that git leaves behind after a conflicted rebase was published as a release note. - Rebase artifacts that ended up as commit subjects, such as "# This is a combination of 2 commits." and "--fixup=<sha>", were grouped under Other Changes. - A first-time contributor whose handle could not be resolved rendered as "@ made their first contribution". upper_first is no longer applied to list items. It was capitalising identifiers, turning "iroh-relay 1.0:" into "Iroh-relay 1.0:" and "own_url_for_preflight()" into "Own_url_for_preflight()".
Locks in the behaviour fixed in the previous commit: list structure in a commit body, joining hard-wrapped lines, leaving list items as the author wrote them, dropping git comment lines, and skipping rebase artifacts. Verified to fail against the previous configuration, so it is a genuine regression test rather than a description of current behaviour. The test does not push, because changelog generation is entirely local. That means it runs without the Gitea and registry services, unlike the rest of this file.
Pre-existing formatting drift, unrelated to the changelog configuration changes on this branch. Committed separately so it does not obscure them.
The suite was not run in CI at all, so nothing checked the changelog configuration. Most of these tests need a Git server and a crate registry, which this workflow does not provide, but changelog generation is entirely local and needs neither. Move the rendering test into its own file so the workflow can select it by target rather than by test name, and so a future rendering test is picked up without also having to remember to edit the workflow.
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughUpdated git-cliff preprocessing and templates to normalize and render commit bodies, including nested lists, wrapped paragraphs, contributor sections, and filtered artifacts. Added an integration test covering these rendering cases and a workflow step to run it independently. Reformatted unchanged release utility expressions and warning text. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/integration/tests/changelog_rendering.rs (1)
38-40: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a nested-list fixture.
This only tests a continuation line. Add an indented child bullet and assert its deeper output indentation; the current preprocessor flattens nested bullets.
🤖 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 `@crates/integration/tests/changelog_rendering.rs` around lines 38 - 40, Extend the changelog fixture in the relevant rendering test to include an indented nested child bullet, then update assertions to verify the child preserves deeper output indentation rather than being flattened. Keep the existing continuation-line coverage and use the current changelog preprocessing/rendering test flow.
🤖 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 `@pre-1.0-cliff.toml`:
- Line 98: Update the list-item preprocessing rule in pre-1.0-cliff.toml so it
preserves each item's leading indentation in the marker representation, and
ensure the template renders that depth as corresponding indentation. In
crates/integration/tests/changelog_rendering.rs, extend the changelog rendering
fixture with a parent and nested child list item, then assert the rendered child
remains nested.
- Line 109: Update the hash-prefixed matcher in the configuration to match only
the known squash-artifact format rather than every subject beginning with “#”.
Preserve legitimate subjects such as “#123: fix startup” when
filter_unconventional is false, and keep the existing skip behavior for actual
squash artifacts.
---
Nitpick comments:
In `@crates/integration/tests/changelog_rendering.rs`:
- Around line 38-40: Extend the changelog fixture in the relevant rendering test
to include an indented nested child bullet, then update assertions to verify the
child preserves deeper output indentation rather than being flattened. Keep the
existing continuation-line coverage and use the current changelog
preprocessing/rendering test flow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 944d2149-90ea-431a-b430-b41710d09d91
📒 Files selected for processing (6)
.github/workflows/test.yamlcrates/integration/src/lib.rscrates/integration/tests/changelog_rendering.rscrates/release_util/src/lib.rscrates/release_util/src/prepare_release.rspre-1.0-cliff.toml
|
❌ Found 4 compliant commit and 2 non-compliant commits in babad90...4eb1097. Commit 979194c by @ThetaSinner is not conform to the conventional commit specification :
Commit 4eb1097 by @ThetaSinner is not conform to the conventional commit specification :
|
cdunster
left a comment
cdunster
left a comment
There was a problem hiding this comment.
Some comments but looks good
| //! Unlike the other tests in this crate, nothing here pushes to a remote or publishes a crate, | ||
| //! because changelog generation is entirely local. That means this file, and only this file, can | ||
| //! run without the Gitea and registry services, so CI runs it on every pull request. |
There was a problem hiding this comment.
Nitpick: Some of this seems like it might become stale; maybe something like:
| //! Unlike the other tests in this crate, nothing here pushes to a remote or publishes a crate, | |
| //! because changelog generation is entirely local. That means this file, and only this file, can | |
| //! run without the Gitea and registry services, so CI runs it on every pull request. | |
| //! The tests in this module must not rely on external services so that they can be run locally and | |
| //! in CI. |
| /// | ||
| /// Unlike the other tests in this file, this one never pushes, because changelog generation is | ||
| /// purely local. That keeps it runnable without the Gitea and registry services. |
There was a problem hiding this comment.
I think that "file" here is a typo, but I also think that this paragraph can just be removed as it's covered by the module description.
| /// | |
| /// Unlike the other tests in this file, this one never pushes, because changelog generation is | |
| /// purely local. That keeps it runnable without the Gitea and registry services. |
| /// Unlike the other tests in this file, this one never pushes, because changelog generation is | ||
| /// purely local. That keeps it runnable without the Gitea and registry services. | ||
| #[test] | ||
| fn commit_body_rendering() { |
There was a problem hiding this comment.
I recommend splitting each of these into separate tests so we can more easily see if a single feature breaks. I do also get wanting to test a larger file though, so it's not a strong request.
| " - Some lead-in prose that is hard wrapped over two lines: | ||
| - an item that is itself hard wrapped onto a second line | ||
| - rcgen 0.14: CertifiedKey::key_pair renamed to signing_key | ||
| - A trailing paragraph after the list." |
There was a problem hiding this comment.
This trailing paragraph renders as if it is a separate item/commit. This could be a problem, as I've seen some commits that write this as a continuation, e.g.
The commit:
feat: Some feature that changes the API to add a field `foo`
This is because `bar()` needs it.
## Features
- Some feature that changes the API to add a field `foo`
- This is because `bar()` needs it.Now the body that was adding more detail seems like a new item and doesn't make sense.
| // The Other Changes group itself is not checked for absence. It legitimately holds the | ||
| // issue-reference subject above, which is an unconventional commit rather than an artifact. | ||
| // The assertions above already establish that no artifact reaches the changelog at all. | ||
| } |
There was a problem hiding this comment.
I'd quite like to see a test for rendering breaking changes with a reason, so a commit message like:
feat!: added the `foo` field to the `Bar` struct
The `foo` field is required by the following callers:
- `use_foo_1()`
- `use_foo_2()`
BREAKING CHANGE: This is a change to the public API, and all callers now need to handle `foo`.
This would hopefully render as something like:
- [**BREAKING**] Added the `foo` field to the `Bar` struct
- The `foo` field is required by the following callers:
- `use_foo_1()`
- `use_foo_2()`
- **Breaking Change**: This is a change to the public API, and all callers now need to handle `foo`.2 participants
Summary
# Conflicts:block git leaves behind after a conflicted rebase is no longer published as a release note.# This is a combination of 2 commits.and--fixup=<sha>, are dropped rather than grouped under Other Changes.@ made their first contribution. If that leaves no named contributors at all, the### First-time Contributorsheading is dropped too rather than standing over an empty list.upper_firstno longer applies to list items, where it was capitalising identifiers:iroh-relay 1.0:becameIroh-relay 1.0:,own_url_for_preflight()becameOwn_url_for_preflight().All five were visible in the kitsune2
0.5.0notes.Checked against kitsune2's full history and against
hc-http-gw,hc-chc-serviceandhc-spin-rust-utils. The last is byte-identical to the old output; the other two change only as described above.The suite was not run in CI at all, so nothing checked this configuration. Most of it needs a Git server and a crate registry, but changelog generation is local, so the rendering test is split into its own target and the workflow now runs it. It was verified to fail against the previous configuration.
The formatting commit is pre-existing drift, kept separate.
Before merging
prepare-release.ymlfetches this file fromrefs/heads/main, so merging takes effect for every repository's next release immediately, with no version pinning.