Implement Cargo manifest version bumping - #10
Conversation
Reviewer's GuideImplements the bump command to propagate a specified version across workspace and crate Cargo.toml files using tomlkit, updates the CLI interface to accept a version argument, and revises tests and documentation to cover the new behavior. Sequence diagram for the new bump command workflowsequenceDiagram
actor User
participant CLI
participant "bump.run()"
participant "WorkspaceGraph"
participant "tomlkit"
User->>CLI: bump <version>
CLI->>"bump.run()": run(workspace_root, target_version)
"bump.run()"->>"WorkspaceGraph": load_workspace(root_path)
"bump.run()"->>"tomlkit": _update_manifest(workspace_manifest, selectors, target_version)
loop For each crate not in exclude
"bump.run()"->>"tomlkit": _update_manifest(crate.manifest_path, ("package",), target_version)
end
"bump.run()"-->>CLI: Return summary message
CLI-->>User: Print summary (e.g. "Updated version to ...")
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughImplements a real bump command requiring a semantic version argument, validates it at the CLI layer before loading workspace/configuration, updates workspace and member Cargo.toml versions via tomlkit (honoring Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CLI as lading (CLI)
participant Runner as CLI runner
participant Bump as commands.bump.run
participant Loader as Config/Workspace loaders
participant FS as Cargo.toml files
U->>CLI: lading bump <version> [--workspace-root]
CLI->>CLI: validate version format (major.minor.patch, allow prerelease/build)
CLI->>Runner: invoke bump with normalized root and version
Runner->>Bump: run(workspace_root, target_version, configuration?, workspace?)
alt configuration/workspace omitted
Bump->>Loader: load configuration & workspace graph
Loader-->>Bump: config, workspace
end
loop each selected manifest (workspace + non-excluded members)
Bump->>FS: read manifest via tomlkit
alt already target version
Bump-->>Bump: skip write
else update required
Bump->>FS: write updated manifest atomically (preserve trivia/comments)
end
end
Bump-->>Runner: return "Updated version to X in N manifest(s)." or "No manifest changes required"
Runner-->>CLI: display summary to user
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
🧬 Code graph analysis (1)lading/commands/bump.py (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The BDD step definitions reference
parse_tomlbut don’t import it—addfrom tomlkit import parse as parse_tomlto avoid a NameError. - Add a unit or BDD test for the code path when all versions already match the target, asserting the “No manifest changes required; all versions already X” message.
- Since
bump.runno longer usesbump.doc_files, consider removing or repurposing that config field to reduce confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The BDD step definitions reference `parse_toml` but don’t import it—add `from tomlkit import parse as parse_toml` to avoid a NameError.
- Add a unit or BDD test for the code path when all versions already match the target, asserting the “No manifest changes required; all versions already X” message.
- Since `bump.run` no longer uses `bump.doc_files`, consider removing or repurposing that config field to reduce confusion.
## Individual Comments
### Comment 1
<location> `lading/cli.py:171` </location>
<code_context>
@app.command
def bump(
+ version: str,
workspace_root: WorkspaceRootOption | None = None,
) -> str:
</code_context>
<issue_to_address>
**suggestion:** No validation for version string format.
Consider adding input validation to ensure the version argument matches the expected format before processing.
</issue_to_address>
### Comment 2
<location> `lading/commands/bump.py:27` </location>
<code_context>
+)
+
def run(
- workspace_root: Path,
+ workspace_root: Path | str,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural tests for the new bump command logic to ensure correct manifest version propagation.
The new implementation for the bump command updates manifest versions and respects exclusions. Behavioural tests are required to verify that the command correctly updates the intended files and produces the expected summary output.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 3
<location> `lading/commands/bump.py:60` </location>
<code_context>
+ return f"Updated version to {target_version} in {changed} manifest(s)."
+
+
+def _update_manifest(
+ manifest_path: Path,
+ selectors: tuple[tuple[str, ...], ...],
</code_context>
<issue_to_address>
**issue (review_instructions):** Add unit tests for _update_manifest to verify correct version assignment and file writing.
Unit tests should cover cases where the manifest is updated, not updated (already matches), and where selectors do not match any table. This ensures the function behaves correctly in all scenarios.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 4
<location> `lading/commands/bump.py:82` </location>
<code_context>
+ return parse_toml(content)
+
+
+def _select_table(
+ document: TOMLDocument | Table,
+ keys: tuple[str, ...],
</code_context>
<issue_to_address>
**issue (review_instructions):** Add unit tests for _select_table to ensure correct table selection and handling of missing keys.
Unit tests should verify that _select_table returns the correct table for valid selectors and None for invalid or missing keys.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 5
<location> `lading/commands/bump.py:99` </location>
<code_context>
+ return current if isinstance(current, Table) else None
+
+
+def _assign_version(table: Table | None, target_version: str) -> bool:
+ """Update ``table['version']`` when ``table`` is present."""
+ if table is None:
</code_context>
<issue_to_address>
**issue (review_instructions):** Add unit tests for _assign_version to check version assignment logic.
Unit tests should cover cases where the table is None, the version matches, and the version needs updating.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 6
<location> `lading/commands/bump.py:110` </location>
<code_context>
+ return True
+
+
+def _value_matches(value: object, expected: str) -> bool:
+ """Return ``True`` when ``value`` already equals ``expected``."""
+ sentinel = object()
</code_context>
<issue_to_address>
**issue (review_instructions):** Add unit tests for _value_matches to ensure correct comparison logic.
Unit tests should verify that _value_matches returns True when the value matches the expected string, including cases where the value has a 'value' attribute.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 7
<location> `lading/commands/bump.py:114-116` </location>
<code_context>
def _value_matches(value: object, expected: str) -> bool:
"""Return ``True`` when ``value`` already equals ``expected``."""
sentinel = object()
attribute = getattr(value, "value", sentinel)
if attribute is not sentinel:
return attribute == expected
return value == expected
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Lift code into else after jump in control flow ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
- Swap if/else branches ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
- Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
```suggestion
return value == expected if attribute is sentinel else attribute == expected
```
</issue_to_address>
### Comment 8
<location> `tests/bdd/steps/test_cli_steps.py:105` </location>
<code_context>
def _run_cli(
repo_root: Path,
workspace_directory: Path,
*command_args: str,
) -> dict[str, typ.Any]:
command = [
sys.executable,
"-m",
"lading.cli",
"--workspace-root",
str(workspace_directory),
]
command.extend(command_args)
completed = subprocess.run( # noqa: S603
command,
check=False,
cwd=str(repo_root),
capture_output=True,
text=True,
)
return {
"returncode": completed.returncode,
"stdout": completed.stdout,
"stderr": completed.stderr,
"workspace": workspace_directory.resolve(),
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge extend into list declaration ([`merge-list-extend`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-list-extend/))
```suggestion
*command_args,
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lading/cli.py (1)
170-173: Consider adding a Parameter annotation for better CLI help.The
versionparameter lacks aParameterannotation with help text. While cyclopts can infer basic help from the parameter name and docstring, an explicit annotation would improve consistency withworkspace_rootand provide clearer user guidance.Consider applying this diff:
+_VERSION_PARAMETER = Parameter( + help="Target semantic version (e.g., 1.2.3) to set across workspace manifests.", +) +VersionArgument = typ.Annotated[str, _VERSION_PARAMETER] + @app.command def bump( - version: str, + version: VersionArgument, workspace_root: WorkspaceRootOption | None = None, ) -> str:lading/commands/bump.py (2)
82-96: Consider simplifying the final type check.The function correctly navigates nested tables, but the final check on line 96 could be clearer. Since
currentis updated only when values areTableinstances, the finalisinstancecheck is needed only for the empty-keys edge case (wherecurrentremains aTOMLDocument).Consider adding a comment explaining why the final check is necessary:
current = next_value -return current if isinstance(current, Table) else None +# Empty keys tuple leaves current as TOMLDocument; verify it's a Table +return current if isinstance(current, Table) else NoneAlternatively, explicitly handle the empty-keys case at the start:
def _select_table( document: TOMLDocument | Table, keys: tuple[str, ...], ) -> Table | None: """Return the nested table located by ``keys`` if it exists.""" + if not keys: + return document if isinstance(document, Table) else None current: object = document
110-116: Consider usinghasattrfor clarity.The sentinel pattern is correct but could be simplified with
hasattrfor slightly improved readability.def _value_matches(value: object, expected: str) -> bool: """Return ``True`` when ``value`` already equals ``expected``.""" - sentinel = object() - attribute = getattr(value, "value", sentinel) - if attribute is not sentinel: - return attribute == expected + if hasattr(value, "value"): + return value.value == expected # type: ignore[attr-defined] return value == expected
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
docs/lading-design.md(1 hunks)docs/roadmap.md(1 hunks)docs/usage-guide.md(2 hunks)lading/cli.py(1 hunks)lading/commands/bump.py(1 hunks)tests/bdd/features/cli.feature(2 hunks)tests/bdd/steps/test_cli_steps.py(5 hunks)tests/unit/test_bump_command.py(1 hunks)tests/unit/test_cli.py(6 hunks)tests/unit/test_commands_placeholder.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*.py: Python files must use snake_case filenames (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants must use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single leading underscore
Use typing everywhere and maintain full static type coverage
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only usage
Avoid Any; prefer precise types (TypeVar, Protocol, Literal, Union); use typing.cast only when necessary with justification; use object for unknown opaque values
Be explicit with return types (e.g., -> None, -> str) for all public functions and methods
Favor immutability: prefer tuples to lists and MappingProxyType for read-only mappings; document third-party frozendict if used
Use# pyright: ignoresparingly and include an explanation comment when used
Avoid side effects at import time (no global state mutation or actions on import)
Use docstrings (NumPy format) for public functions, classes, and modules
Explain tricky code with inline comments
**/*.py: Prefer context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual try/finally blocks
Use contextlib.contextmanager (@contextmanager) to implement simple, linear setup/teardown context managers
Implement a class with enter and exit for context managers that require internal state or more complex lifecycle handling
For file I/O, prefer with open(...) as ... over open()/try/finally/close patterns
Choose @contextmanager when control flow is linear and no persistent state is needed
Choose a class-based context manager when there is internal state, lifecycle methods, re-entry, or advanced context needs
**/*.py: Exception classes must end with the suffix 'Error' (N818)
Prefer specific built-ins (e.g., TypeError, ValueError) or domain exceptions over raising Exception directly (TRY003/TRY004)
Preserve causal chains...
Files:
tests/unit/test_bump_command.pylading/commands/bump.pytests/unit/test_cli.pylading/cli.pytests/unit/test_commands_placeholder.pytests/bdd/steps/test_cli_steps.py
{**/unittests/test_*.py,tests/**/*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/**/*.py}: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective: test public behaviour, not internals
Files:
tests/unit/test_bump_command.pytests/unit/test_cli.pytests/unit/test_commands_placeholder.pytests/bdd/steps/test_cli_steps.py
tests/**/*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, assert specific exception types (and optional message via regex) rather than using broad Exception (B017)
Files:
tests/unit/test_bump_command.pytests/unit/test_cli.pytests/unit/test_commands_placeholder.pytests/bdd/steps/test_cli_steps.py
{README.md,docs/**}
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate documentation: keep README.md or a docs/ directory near reusable packages and include usage examples
Files:
docs/lading-design.mddocs/usage-guide.mddocs/roadmap.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use markdown files in docs/ as the knowledge base and source of truth for requirements, dependencies, and architectural decisions.
Proactively update relevant docs/ markdown when decisions, requirements, dependencies, or architecture change.
Files:
docs/lading-design.mddocs/usage-guide.mddocs/roadmap.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown files must pass markdownlint.
Markdown files containing Mermaid diagrams must pass nixie validation.
Files:
docs/lading-design.mddocs/usage-guide.mddocs/roadmap.md
🧬 Code graph analysis (6)
tests/unit/test_bump_command.py (3)
lading/workspace/models.py (2)
WorkspaceCrate(30-40)WorkspaceGraph(43-52)lading/config.py (2)
LadingConfig(84-105)BumpConfig(36-55)lading/commands/bump.py (1)
run(27-57)
lading/commands/bump.py (4)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/config.py (2)
LadingConfig(84-105)current_configuration(157-163)lading/workspace/models.py (2)
WorkspaceGraph(43-52)load_workspace(55-62)lading/cli.py (1)
bump(170-184)
tests/unit/test_cli.py (3)
lading/cli.py (3)
main(112-147)bump(170-184)publish(188-193)lading/config.py (1)
LadingConfig(84-105)lading/workspace/models.py (1)
WorkspaceGraph(43-52)
lading/cli.py (2)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/commands/bump.py (1)
run(27-57)
tests/unit/test_commands_placeholder.py (3)
lading/cli.py (1)
publish(188-193)lading/commands/bump.py (1)
run(27-57)lading/commands/publish.py (1)
run(18-36)
tests/bdd/steps/test_cli_steps.py (1)
tests/conftest.py (1)
repo_root(16-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (25)
docs/roadmap.md (1)
89-89: LGTM! Step 2.1 completion accurately reflected.The roadmap update correctly marks the workspace and member version update task as complete, aligning with the bump command implementation delivered in this PR.
docs/lading-design.md (1)
280-290: LGTM! Implementation notes accurately document Step 2.1 behavior.The notes clearly describe the manifest rewriting approach (tomlkit for format preservation), exclusion handling, and user-facing output format, all of which align with the implementation in
lading/commands/bump.py.lading/cli.py (1)
176-184: LGTM! Lambda correctly adapts the runner signature.The lambda wrapper properly captures
versionfrom the enclosing scope and passes it as the second positional argument tocommands.bump.run, aligning with the updated signature that now requirestarget_version.tests/unit/test_commands_placeholder.py (2)
11-11: LGTM! Correctly removed bump placeholder imports.The import cleanup is appropriate now that bump has a full implementation with dedicated test coverage in
tests/unit/test_bump_command.py.
50-96: LGTM! Test parametrization correctly scoped to publish only.The parametrized tests now appropriately focus on the remaining placeholder (publish), and the dedicated
test_bump_run_mentions_commandhas been removed since bump is no longer a placeholder.tests/unit/test_cli.py (2)
23-31: LGTM! Test data structure clearly models version-driven behavior.The rename from
placeholder_texttoreturn_valueis more precise, and the newexpected_versionfield enables validation of version propagation through the CLI to the bump command.
142-168: LGTM! Test assertions correctly validate version flow.The test properly validates that:
- For bump:
workspace_rootandversionare passed as positional args, withconfigurationandworkspaceas kwargs- For publish: all three args are passed positionally by
_run_with_context- The version value flows through to the expected destination
tests/bdd/features/cli.feature (2)
2-8: LGTM! BDD scenario accurately reflects version-driven bump behavior.The updated scenario name, invocation with version argument, and new assertions for manifest version updates correctly capture the Step 2.1 implementation.
17-18: LGTM! Missing configuration scenario updated for version requirement.The scenario correctly includes the version argument for the bump invocation.
tests/unit/test_bump_command.py (7)
18-28: LGTM! Helper creates clean workspace manifests.The helper correctly writes a minimal workspace manifest with members list and workspace.package.version, suitable for testing version updates.
31-56: LGTM! Workspace graph builder is well-structured.The helper creates a representative workspace with two member crates, each with its own manifest, properly setting up the test fixture for bump command validation.
59-65: LGTM! Version loader appropriately uses tomlkit.Reading versions with
tomlkit.parsealigns with how the implementation updates manifests, ensuring test assertions validate the actual file state.Based on learnings
74-84: LGTM! Test validates core manifest update behavior.The test correctly verifies that
bump.run:
- Updates the workspace manifest
workspace.package.version- Updates each member crate's
package.version- Returns an accurate summary message
87-96: LGTM! Test validates exclusion logic.The test correctly verifies that crates in
bump.excluderetain their original version while others are updated, ensuring the exclusion feature works as designed.
99-110: LGTM! Test validates workspace root normalization.The test correctly verifies that relative workspace paths are resolved before applying updates, ensuring consistent behavior regardless of how the path is provided.
113-122: LGTM! Test validates configuration and workspace loading.The test correctly verifies that when configuration and workspace are omitted,
bump.runfalls back to loading them viacurrent_configuration()andload_workspace(), ensuring the command works in both programmatic and CLI contexts.docs/usage-guide.md (1)
68-85: LGTM! Usage guide accurately documents bump behavior.The updated documentation clearly explains:
- Version is a required positional argument
- What manifests get updated (workspace.package.version and member package.version)
- Exclusion behavior via
bump.exclude- The concise summary output format
All details align with the implementation in
lading/commands/bump.py.lading/commands/bump.py (3)
7-8: LGTM: tomlkit imports.The import of
parseandTableis appropriate for manipulating TOML while preserving formatting.Based on learnings: tomlkit 0.13.3 provides style-preserving TOML parsing via its DOM API, which is exactly what's needed here.
21-24: LGTM: Workspace selectors correctly target Cargo manifest structures.The selectors appropriately handle both standalone package sections and workspace-scoped package sections, aligning with Cargo's manifest schema.
42-57: LGTM: Main logic correctly updates manifests and respects exclusions.The implementation properly counts changes, honors the exclusion list from configuration, and provides clear user feedback.
tests/bdd/steps/test_cli_steps.py (5)
16-17: LGTM: Test imports properly support BDD parsing and TOML validation.The addition of
parsersfor parameterized steps andparse_tomlfor manifest verification is appropriate.
55-64: LGTM: Workspace manifest creation aligns with Cargo workspace format.The test fixture correctly creates a workspace manifest with
[workspace.package]version, matching Cargo's schema for workspace-level package metadata.
93-118: LGTM: CLI runner generalized to support multiple arguments.The change from a single subcommand string to variadic
*command_argsimproves flexibility for testing commands with parameters.
121-131: LGTM: Bump step correctly parses and passes version parameter.The use of
parsers.parseto extract the version from the step text and pass it through to_run_cliis correct.
143-172: LGTM: New assertion steps properly validate manifest updates.The three new
thensteps correctly:
- Verify the command output message contains the expected version
- Parse and validate the workspace manifest
[workspace.package].version- Parse and validate individual crate manifest
[package].version
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved?
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
docs/usage-guide.md (1)
67-75: Tighten grammar and wording in bump usage section.A few minor fixes improve clarity and markdownlint style.
Apply this diff:
-`bump` synchronises manifest versions across the workspace. The command -requires the target version as a positional argument and rejects inputs that do -not match the `<major>.<minor>.<patch>` semantic version shape. All validation -happens before the command loads workspace metadata so mistakes fail fast. +`bump` synchronises manifest versions across the workspace. The command +requires the target version as a positional argument and rejects inputs that do +not match the `<major>.<minor>.<patch>` semantic version pattern. All validation +happens before the command loads workspace metadata, so mistakes fail fast. @@ -Running the command updates: +Running the command updates: @@ -`lading` prints a short summary such as -`Updated version to 1.2.3 in 3 manifest(s).` so that release automation can -assert the change without parsing files directly. When every manifest already -records the requested version the CLI instead reports +`lading` prints a short summary such as +`Updated version to 1.2.3 in 3 manifest(s).`, so that release automation can +assert the change without parsing files directly. When every manifest already +records the requested version, the CLI instead reports `No manifest changes required; all versions already 1.2.3.`Also applies to: 80-91
lading/cli.py (1)
170-181: Consider broader SemVer acceptance (pre-release/build).Current regex only accepts X.Y.Z. If you plan to support SemVer 2.0 pre-release/build (e.g., 1.2.3-alpha.1+meta), expand the validator later.
Example pattern to consider when needed:
- r'^\d+.\d+.\d+(?:-[0-9A-Za-z.-]+)?(?:+[0-9A-Za-z.-]+)?$'
docs/lading-design.md (1)
275-292: Minor grammar/punctuation improvements in Step 2.1 notes.Polish for readability.
Apply:
-implementation updates both `[package]` and `[workspace.package]` sections in -the workspace manifest when present. +The implementation updates both `[package]` and `[workspace.package]` sections +in the workspace manifest when present. @@ -When no manifest requires changes the command reports a dedicated +When no manifest requires changes, the command reports a dedicated "No manifest changes required" message instead of rewriting files. @@ -Invalid formats raise a user-facing error without touching the file -system. +Invalid formats raise a user-facing error without touching the filesystem.docs/roadmap.md (1)
89-96: Roadmap updates read well; small style nits optional.Optional punctuation tweaks for consistency.
- - **Completion Criteria:** An integration test confirms that running + - **Completion Criteria:** An integration test confirms that running `lading bump 1.2.3` on a fixture workspace results in all `package.version` fields being updated to `1.2.3`. @@ - - **Outcome:** The `bump` command updates version numbers within TOML code - fences in documentation files using new configuration introduced alongside - this feature. + - **Outcome:** The `bump` command updates version numbers within TOML code + fences in documentation files, using new configuration introduced alongside + this feature.Also applies to: 124-127
tests/unit/test_bump_command.py (1)
74-85: Add a test for root[package]updates in the workspace manifest.Current coverage verifies
[workspace.package], but not a root[package]entry. Add a case where the root manifest contains[package]and ensure it updates to the target version.I can draft the additional test if helpful.
tests/bdd/features/cli.feature (1)
1-16: Add a scenario for invalid version input.Cover CLI validation by asserting the error and exit code for an invalid version (e.g., 1.2).
Example:
Scenario: Bumping with an invalid version fails fast Given a workspace directory with configuration And cargo metadata describes a sample workspace When I invoke lading bump 1.2 with that workspace Then the CLI exits with code 1 And the stderr contains "Invalid version argument"Also applies to: 23-26
lading/commands/bump.py (1)
60-74: Ensure UTF‑8 and prefer atomic writes to avoid partial manifests.Be explicit about encoding and write atomically to prevent truncated files on failure.
Apply:
- document = _parse_manifest(manifest_path) + document = _parse_manifest(manifest_path) @@ - if changed: - manifest_path.write_text(document.as_string()) + if changed: + content = document.as_string() + # Atomic write + import os, tempfile + dirpath = manifest_path.parent + fd, tmp = tempfile.mkstemp(dir=dirpath, prefix=manifest_path.name + ".", text=True) + try: + with os.fdopen(fd, "w", encoding="utf-8", newline="") as fh: + fh.write(content) + os.replace(tmp, manifest_path) + finally: + try: + os.unlink(tmp) + except FileNotFoundError: + pass return changedAnd make reads explicit UTF‑8:
-def _parse_manifest(manifest_path: Path) -> TOMLDocument: +def _parse_manifest(manifest_path: Path) -> TOMLDocument: @@ - content = manifest_path.read_text() + content = manifest_path.read_text(encoding="utf-8") return parse_toml(content)Also applies to: 76-80
tests/unit/test_cli.py (2)
153-167: Also assert configuration is passed to bump.run for parity.Bump path verifies workspace/version; consider asserting configuration was provided too, like publish does.
Apply this diff to strengthen the assertion:
if case.command_module is bump_command: workspace_root_arg, version_arg = captured_args assert workspace_root_arg == tmp_path.resolve() assert version_arg == case.expected_version + assert "configuration" in captured_kwargs + assert isinstance(captured_kwargs["configuration"], config_module.LadingConfig) workspace_model = captured_kwargs["workspace"]
266-282: Also assert configuration is wired through Cyclopts app.You already validate workspace and version; add a check for configuration as well.
Apply this diff to add the assertion:
def fake_run( workspace_root: Path, version: str, *, configuration: config_module.LadingConfig, workspace: WorkspaceGraph, ) -> str: assert workspace_root == tmp_path.resolve() assert version == "4.5.6" + assert isinstance(configuration, config_module.LadingConfig) assert workspace is graph return "bump summary"tests/bdd/steps/test_cli_steps.py (3)
33-48: Add precondition checks for manifest existence.These steps assume manifests exist (created by another Given). To make failures clearer if steps are reordered/missed, assert files exist before reading.
Apply this diff:
def given_workspace_versions_match( workspace_directory: Path, version: str, ) -> None: """Ensure the workspace and member manifests record ``version``.""" workspace_manifest = workspace_directory / "Cargo.toml" + if not workspace_manifest.exists(): + raise AssertionError(f"Workspace manifest not found: {workspace_manifest}") workspace_document = parse_toml(workspace_manifest.read_text()) workspace_document["workspace"]["package"]["version"] = version workspace_manifest.write_text(workspace_document.as_string()) crate_manifest = workspace_directory / "crates" / "alpha" / "Cargo.toml" + if not crate_manifest.exists(): + raise AssertionError(f"Crate manifest not found: {crate_manifest}") crate_document = parse_toml(crate_manifest.read_text()) crate_document["package"]["version"] = version crate_manifest.write_text(crate_document.as_string())
70-79: Dedent embedded TOML to avoid leading whitespace.Indentation in triple-quoted TOML can be fragile. Dedent before writing for clarity/portability.
Apply this diff:
- workspace_manifest.write_text( - """ - [workspace] - members = ["crates/alpha"] - - [workspace.package] - version = "0.1.0" - """ - ) + workspace_manifest.write_text( + textwrap.dedent( + """ + [workspace] + members = ["crates/alpha"] + + [workspace.package] + version = "0.1.0" + """ + ).lstrip() + )And add the import at the top of the file:
import textwrap
158-175: Consider less brittle output assertions.Exact full-line matches can break on minor phrasing tweaks. Matching key fragments reduces churn.
Apply this diff:
- assert f"Updated version to {version}" in stdout + assert "Updated version to " in stdout and version in stdout- assert f"No manifest changes required; all versions already {version}." in stdout + assert "No manifest changes required" in stdout and f"already {version}" in stdout
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
docs/lading-design.md(2 hunks)docs/roadmap.md(2 hunks)docs/usage-guide.md(2 hunks)lading/cli.py(2 hunks)lading/commands/bump.py(1 hunks)lading/config.py(1 hunks)tests/bdd/features/cli.feature(2 hunks)tests/bdd/steps/test_cli_steps.py(6 hunks)tests/conftest.py(0 hunks)tests/unit/test_bump_command.py(1 hunks)tests/unit/test_cli.py(5 hunks)tests/unit/test_commands_placeholder.py(3 hunks)tests/unit/test_config.py(0 hunks)
💤 Files with no reviewable changes (2)
- tests/unit/test_config.py
- tests/conftest.py
✅ Files skipped from review due to trivial changes (1)
- lading/config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/test_commands_placeholder.py
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*.py: Python files must use snake_case filenames (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants must use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single leading underscore
Use typing everywhere and maintain full static type coverage
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only usage
Avoid Any; prefer precise types (TypeVar, Protocol, Literal, Union); use typing.cast only when necessary with justification; use object for unknown opaque values
Be explicit with return types (e.g., -> None, -> str) for all public functions and methods
Favor immutability: prefer tuples to lists and MappingProxyType for read-only mappings; document third-party frozendict if used
Use# pyright: ignoresparingly and include an explanation comment when used
Avoid side effects at import time (no global state mutation or actions on import)
Use docstrings (NumPy format) for public functions, classes, and modules
Explain tricky code with inline comments
**/*.py: Prefer context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual try/finally blocks
Use contextlib.contextmanager (@contextmanager) to implement simple, linear setup/teardown context managers
Implement a class with enter and exit for context managers that require internal state or more complex lifecycle handling
For file I/O, prefer with open(...) as ... over open()/try/finally/close patterns
Choose @contextmanager when control flow is linear and no persistent state is needed
Choose a class-based context manager when there is internal state, lifecycle methods, re-entry, or advanced context needs
**/*.py: Exception classes must end with the suffix 'Error' (N818)
Prefer specific built-ins (e.g., TypeError, ValueError) or domain exceptions over raising Exception directly (TRY003/TRY004)
Preserve causal chains...
Files:
tests/unit/test_cli.pytests/unit/test_bump_command.pytests/bdd/steps/test_cli_steps.pylading/commands/bump.pylading/cli.py
{**/unittests/test_*.py,tests/**/*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/**/*.py}: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective: test public behaviour, not internals
Files:
tests/unit/test_cli.pytests/unit/test_bump_command.pytests/bdd/steps/test_cli_steps.py
tests/**/*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, assert specific exception types (and optional message via regex) rather than using broad Exception (B017)
Files:
tests/unit/test_cli.pytests/unit/test_bump_command.pytests/bdd/steps/test_cli_steps.py
{README.md,docs/**}
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate documentation: keep README.md or a docs/ directory near reusable packages and include usage examples
Files:
docs/lading-design.mddocs/usage-guide.mddocs/roadmap.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use markdown files in docs/ as the knowledge base and source of truth for requirements, dependencies, and architectural decisions.
Proactively update relevant docs/ markdown when decisions, requirements, dependencies, or architecture change.
Files:
docs/lading-design.mddocs/usage-guide.mddocs/roadmap.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown files must pass markdownlint.
Markdown files containing Mermaid diagrams must pass nixie validation.
Files:
docs/lading-design.mddocs/usage-guide.mddocs/roadmap.md
🧬 Code graph analysis (5)
tests/unit/test_cli.py (3)
lading/cli.py (2)
main(113-148)publish(203-208)lading/config.py (1)
LadingConfig(82-103)lading/workspace/models.py (1)
WorkspaceGraph(43-52)
tests/unit/test_bump_command.py (3)
lading/workspace/models.py (2)
WorkspaceCrate(30-40)WorkspaceGraph(43-52)lading/config.py (2)
LadingConfig(82-103)BumpConfig(36-53)lading/commands/bump.py (5)
run(27-57)_update_manifest(60-73)_select_table(82-96)_assign_version(99-107)_value_matches(110-114)
tests/bdd/steps/test_cli_steps.py (1)
tests/conftest.py (1)
repo_root(16-18)
lading/commands/bump.py (4)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/config.py (2)
LadingConfig(82-103)current_configuration(155-161)lading/workspace/models.py (2)
WorkspaceGraph(43-52)load_workspace(55-62)lading/cli.py (1)
bump(184-199)
lading/cli.py (2)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/commands/bump.py (1)
run(27-57)
🪛 LanguageTool
docs/lading-design.md
[grammar] ~282-~282: There might be a mistake here.
Context: ...version value during the update pass. - The command reports a concise summary (...
(QB_NEW_EN)
[grammar] ~286-~286: There might be a mistake here.
Context: ...red" message instead of rewriting files. - Version arguments are validated at the C...
(QB_NEW_EN)
[grammar] ~289-~289: There might be a mistake here.
Context: ...rror without touching the file system. - The bump.doc_files configuration knob ...
(QB_NEW_EN)
docs/usage-guide.md
[grammar] ~69-~69: There might be a mistake here.
Context: ...` semantic version shape. All validation happens before the command loads workspa...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...validation, bump touches the workspace Cargo.toml alongside every member crat...
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...r crate unless the crate name appears in bump.exclude within lading.toml. ``...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ... lading prints a short summary such as `Updated version to 1.2.3 in 3 manifest(...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...fest(s).` so that release automation can assert the change without parsing files ...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...es directly. When every manifest already records the requested version the CLI in...
(QB_NEW_EN)
docs/roadmap.md
[grammar] ~127-~127: There might be a mistake here.
Context: ...n introduced alongside this feature. - Completion Criteria: A test case with ...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (15)
lading/cli.py (2)
189-199: Command wiring and validation look solid.Version is validated, workspace normalised, and run dispatched with configuration/workspace context.
151-168: No changes required: fallback uses existing load_configuration
config.load_configuration is implemented in lading/config.py and delegates to build_loader under the hood, so the exception branch is correct.Likely an incorrect or invalid review comment.
lading/commands/bump.py (1)
21-24: Core bump logic and selectors look correct.
- Updates both
[package]and[workspace.package].- Honors bump.exclude.
- Skips rewrites when versions already match and returns concise messages.
Also applies to: 43-58
tests/unit/test_cli.py (6)
29-31: Good extension of the test case model.Adding return_value and optional expected_version makes the dispatch tests clearer and more flexible.
119-122: CLI args updated correctly for bump.Including the version argument and workspace root here mirrors the new CLI contract well.
142-145: Clean call capture.fake_run signature and capture of args/kwargs are simple and robust.
197-197: No issues.Argument order updated to include version looks correct.
235-235: No issues.Invocation adjusted to include version for exception path is consistent.
241-257: Great: explicit invalid-version test.Ensures CLI validation catches non-SemVer inputs and prevents run() from being invoked.
tests/bdd/steps/test_cli_steps.py (6)
16-18: Needed imports look good.pytest-bdd step helpers and tomlkit parser are correctly imported.
29-29: Config fixture content OK.Minimal config includes publish.strip_patches; matches test expectations.
111-112: Varargs CLI wrapper is cleaner.Switching _run_cli to accept *command_args simplifies all step definitions.
Also applies to: 119-119
136-147: Version threaded through the bump step.BDD step captures and forwards the version correctly.
177-187: Workspace manifest verification looks good.Using tomlkit.parse and reading workspace.package.version is appropriate.
189-199: Crate manifest verification looks good.Straightforward check of package.version with tomlkit is correct.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 891 🏁 Script executed: Length of output: 7755 @leynos Yes, all three issues have been resolved:
All concerns have been addressed in the current implementation. |
|
Re: Could we do the following? I don't know if that is any clearer, but I tend to see chains of isinstance and hasattr as a code smell |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1681 @leynos, I appreciate the pattern-matching approach, but there are a few issues with the proposed implementation:
Here's a cleaner alternative using a from typing import Protocol, runtime_checkable
@runtime_checkable
class ValueWrapper(Protocol):
"""Protocol for tomlkit value wrappers."""
value: str
def _value_matches(value: object, expected: str) -> bool:
"""Return ``True`` when ``value`` already equals ``expected``."""
if isinstance(value, ValueWrapper):
return value.value == expected
return value == expectedThis approach:
The Would you like me to generate this change for you? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1546 🏁 Script executed: Length of output: 3808 🏁 Script executed: Length of output: 3638 @leynos Your proposed solution has a few issues that prevent it from working:
However, your concern about Option 1: Use actual tomlkit types from tomlkit.items import Item
def _value_matches(value: object, expected: str) -> bool:
"""Return ``True`` when ``value`` already equals ``expected``."""
if isinstance(value, Item):
return value.value == expected # type: ignore[attr-defined]
return value == expectedOption 2: Pattern matching with guard clauses def _value_matches(value: object, expected: str) -> bool:
"""Return ``True`` when ``value`` already equals ``expected``."""
match value:
case str():
return value == expected
case _ if hasattr(value, "value"):
return value.value == expected # type: ignore[attr-defined]
case _:
return value == expectedOption 3: Protocol with isinstance (if you prefer the Protocol approach) class ValueWrapper(typ.Protocol):
@property
def value(self) -> str: ...
def _value_matches(value: object, expected: str) -> bool:
"""Return ``True`` when ``value`` already equals ``expected``."""
# Note: isinstance doesn't check Protocol conformance at runtime
# unless you use @runtime_checkable, but that still requires hasattr checks
if hasattr(value, "value"):
return value.value == expected # type: ignore[attr-defined]
return value == expectedI'd recommend Option 1 - it's explicit, clean, and |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/roadmap.md (1)
121-131: Remove deprecated bump.doc_files references in documentation
- docs/lading-design.md (lines 290–291): remove or update the mention of the removed
bump.doc_filesconfiguration knob to reflect its deprecation.docs/usage-guide.md (1)
128-130: Fix cargo metadata flag spellingUse the correct flag form without a space after
--.Apply:
-`cargo metadata -- format-version 1` using `plumbum`, and returns the parsed +`cargo metadata --format-version 1` using `plumbum`, and returns the parsed
🧹 Nitpick comments (11)
docs/roadmap.md (1)
126-127: Minor grammar/whitespace tweakConsider “using new configuration introduced alongside this feature.” (single space) for smoother phrasing.
Based on static analysis hints
tests/unit/test_bump_command.py (1)
158-175: Add a test to ensure inline comments and formatting are preservedTo lock-in tomlkit’s style-preserving intent, add a case where version has a trailing comment and assert it’s retained after bump.
Example:
def test_update_preserves_inline_comment(tmp_path: Path) -> None: manifest = tmp_path / "Cargo.toml" manifest.write_text('[package]\nversion = "0.1.0" # keep me\n') changed = bump._update_manifest(manifest, (("package",),), "1.2.3") assert changed is True text = manifest.read_text() assert '# keep me' in text # and version actually updated doc = parse_toml(text) assert doc["package"]["version"] == "1.2.3"This will catch regressions if the update path replaces the value node and drops trivia. Based on learnings
Also applies to: 196-207
lading/cli.py (1)
175-186: Confirm version policy (pre-release/build metadata)Current regex rejects “1.2.3-alpha”/“1.2.3+build”. If that’s intentional, ignore. If not, expand the pattern.
Option:
-_VERSION_PATTERN = re.compile(r"^\d+\.\d+\.\d+$") +_VERSION_PATTERN = re.compile( + r"^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$" +)Also consider including the bad value in the error for clarity.
lading/commands/bump.py (2)
114-120: Simplify _value_matches using tomlkit ItemUse isinstance against tomlkit.items.Item for clarity and to avoid direct getattribute.
-def _value_matches(value: object, expected: str) -> bool: - """Return ``True`` when ``value`` already equals ``expected``.""" - if hasattr(value, "value"): - attribute = object.__getattribute__(value, "value") - return attribute == expected - return value == expected +def _value_matches(value: object, expected: str) -> bool: + """Return ``True`` when ``value`` already equals ``expected``.""" + if isinstance(value, Item): + return value.value == expected + return value == expected
11-13: Add Item import for type checks/mutationNeeded for the above changes.
-from tomlkit.items import Table +from tomlkit.items import Table, Itemtests/bdd/features/cli.feature (1)
2-9: Optional: add an exclusion scenarioConsider a scenario verifying that a crate listed in bump.exclude is not updated via CLI end-to-end.
Example steps:
- Given bump.exclude contains "alpha"
- When I invoke lading bump 1.2.3 with that workspace
- Then the crate "alpha" manifest version is "0.1.0"
- And the crate "beta" manifest version is "1.2.3"
docs/usage-guide.md (2)
72-75: Clarify wording (“touches” → “updates”)Slightly clearer phrasing.
-When the version string passes validation, `bump` touches the workspace -`Cargo.toml` alongside every member crate unless the crate name appears in -`bump.exclude` within `lading.toml`. +When the version string passes validation, `bump` updates the workspace +`Cargo.toml` and each member crate’s manifest, unless the crate name appears in +`bump.exclude` within `lading.toml`.
86-91: Tighten summary sentence and punctuationMinor readability improvement.
-`lading` prints a short summary such as -`Updated version to 1.2.3 in 3 manifest(s).`, so that release automation can -assert the change without parsing files directly. When every manifest already -records the requested version, the CLI instead reports -`No manifest changes required; all versions already 1.2.3.` +`lading` prints a short summary, for example: +`Updated version to 1.2.3 in 3 manifest(s).` This lets release automation +assert the change without parsing files directly. When every manifest already +records the requested version, the CLI instead reports: +`No manifest changes required; all versions already 1.2.3.`docs/lading-design.md (2)
266-271: Avoid using raw triple backticks inlineInline “```toml” can be misinterpreted by Markdown renderers. Use descriptive text.
- - For each matching file, scan for TOML code fences (```toml). + - For each matching file, scan for TOML fenced code blocks (three backticks + "toml").
283-291: Tighten Step 2.1 notes punctuationMinor clarity/punctuation tweaks.
-- The command reports a concise summary (`Updated version to … in N - manifest(s).`) so callers can assert success without inspecting the file - system. When no manifest requires changes, the command reports a dedicated - "No manifest changes required" message instead of rewriting files. +- The command reports a concise summary (`Updated version to … in N manifest(s).`) + so callers can assert success without inspecting the filesystem. When no + manifest requires changes, it reports a dedicated "No manifest changes required" + message instead of rewriting files.tests/bdd/steps/test_cli_steps.py (1)
17-19: LGTM: BDD steps updated for versioned bump; add explicit UTF‑8 encoding for file IO
- Steps correctly pass/verify versions and parse manifests via tomlkit.
- Recommend explicit
encoding="utf-8"in read_text/write_text for portability across locales. As per coding guidelines on file I/O, prefer explicit encodings.- config_path.write_text('[bump]\n\n[publish]\nstrip_patches = "all"\n') + config_path.write_text('[bump]\n\n[publish]\nstrip_patches = "all"\n', encoding="utf-8") - workspace_document = parse_toml(workspace_manifest.read_text()) + workspace_document = parse_toml(workspace_manifest.read_text(encoding="utf-8")) - workspace_manifest.write_text(workspace_document.as_string()) + workspace_manifest.write_text(workspace_document.as_string(), encoding="utf-8") - crate_document = parse_toml(crate_manifest.read_text()) + crate_document = parse_toml(crate_manifest.read_text(encoding="utf-8")) - crate_manifest.write_text(crate_document.as_string()) + crate_manifest.write_text(crate_document.as_string(), encoding="utf-8") - workspace_manifest.write_text( + workspace_manifest.write_text( textwrap.dedent( """ [workspace] members = ["crates/alpha"] [workspace.package] version = "0.1.0" """ ).lstrip() - ) + , encoding="utf-8") - document = parse_toml(manifest_path.read_text()) + document = parse_toml(manifest_path.read_text(encoding="utf-8")) - document = parse_toml(manifest_path.read_text()) + document = parse_toml(manifest_path.read_text(encoding="utf-8"))As per coding guidelines
Also applies to: 34-55, 77-88, 117-129, 145-156, 167-186, 200-221
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
docs/lading-design.md(2 hunks)docs/roadmap.md(2 hunks)docs/usage-guide.md(2 hunks)lading/cli.py(3 hunks)lading/commands/bump.py(1 hunks)tests/bdd/features/cli.feature(2 hunks)tests/bdd/steps/test_cli_steps.py(7 hunks)tests/unit/test_bump_command.py(1 hunks)tests/unit/test_cli.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*.py: Python files must use snake_case filenames (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants must use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single leading underscore
Use typing everywhere and maintain full static type coverage
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only usage
Avoid Any; prefer precise types (TypeVar, Protocol, Literal, Union); use typing.cast only when necessary with justification; use object for unknown opaque values
Be explicit with return types (e.g., -> None, -> str) for all public functions and methods
Favor immutability: prefer tuples to lists and MappingProxyType for read-only mappings; document third-party frozendict if used
Use# pyright: ignoresparingly and include an explanation comment when used
Avoid side effects at import time (no global state mutation or actions on import)
Use docstrings (NumPy format) for public functions, classes, and modules
Explain tricky code with inline comments
**/*.py: Prefer context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual try/finally blocks
Use contextlib.contextmanager (@contextmanager) to implement simple, linear setup/teardown context managers
Implement a class with enter and exit for context managers that require internal state or more complex lifecycle handling
For file I/O, prefer with open(...) as ... over open()/try/finally/close patterns
Choose @contextmanager when control flow is linear and no persistent state is needed
Choose a class-based context manager when there is internal state, lifecycle methods, re-entry, or advanced context needs
**/*.py: Exception classes must end with the suffix 'Error' (N818)
Prefer specific built-ins (e.g., TypeError, ValueError) or domain exceptions over raising Exception directly (TRY003/TRY004)
Preserve causal chains...
Files:
tests/unit/test_cli.pylading/cli.pytests/unit/test_bump_command.pytests/bdd/steps/test_cli_steps.pylading/commands/bump.py
{**/unittests/test_*.py,tests/**/*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/**/*.py}: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective: test public behaviour, not internals
Files:
tests/unit/test_cli.pytests/unit/test_bump_command.pytests/bdd/steps/test_cli_steps.py
tests/**/*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, assert specific exception types (and optional message via regex) rather than using broad Exception (B017)
Files:
tests/unit/test_cli.pytests/unit/test_bump_command.pytests/bdd/steps/test_cli_steps.py
{README.md,docs/**}
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate documentation: keep README.md or a docs/ directory near reusable packages and include usage examples
Files:
docs/lading-design.mddocs/usage-guide.mddocs/roadmap.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use markdown files in docs/ as the knowledge base and source of truth for requirements, dependencies, and architectural decisions.
Proactively update relevant docs/ markdown when decisions, requirements, dependencies, or architecture change.
Files:
docs/lading-design.mddocs/usage-guide.mddocs/roadmap.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown files must pass markdownlint.
Markdown files containing Mermaid diagrams must pass nixie validation.
Files:
docs/lading-design.mddocs/usage-guide.mddocs/roadmap.md
🧬 Code graph analysis (5)
tests/unit/test_cli.py (3)
lading/cli.py (2)
main(118-153)publish(208-213)lading/config.py (1)
LadingConfig(82-103)lading/workspace/models.py (1)
WorkspaceGraph(43-52)
lading/cli.py (2)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/commands/bump.py (1)
run(29-59)
tests/unit/test_bump_command.py (4)
lading/cli.py (2)
bump(189-204)publish(208-213)lading/workspace/models.py (2)
WorkspaceCrate(30-40)WorkspaceGraph(43-52)lading/config.py (2)
LadingConfig(82-103)BumpConfig(36-53)lading/commands/bump.py (5)
run(29-59)_update_manifest(62-75)_select_table(84-100)_assign_version(103-111)_value_matches(114-119)
tests/bdd/steps/test_cli_steps.py (1)
tests/conftest.py (1)
repo_root(16-18)
lading/commands/bump.py (4)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/config.py (2)
LadingConfig(82-103)current_configuration(155-161)lading/workspace/models.py (2)
WorkspaceGraph(43-52)load_workspace(55-62)lading/cli.py (1)
bump(189-204)
🪛 LanguageTool
docs/lading-design.md
[grammar] ~282-~282: There might be a mistake here.
Context: ...version value during the update pass. - The command reports a concise summary (...
(QB_NEW_EN)
[grammar] ~286-~286: There might be a mistake here.
Context: ...red" message instead of rewriting files. - Version arguments are validated at the C...
(QB_NEW_EN)
[grammar] ~289-~289: There might be a mistake here.
Context: ...error without touching the filesystem. - The bump.doc_files configuration knob ...
(QB_NEW_EN)
docs/usage-guide.md
[grammar] ~69-~69: There might be a mistake here.
Context: ...semantic version pattern. All validation happens before the command loads workspa...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...validation, bump touches the workspace Cargo.toml alongside every member crat...
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...r crate unless the crate name appears in bump.exclude within lading.toml. ``...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ... lading prints a short summary such as `Updated version to 1.2.3 in 3 manifest(...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...est(s).`, so that release automation can assert the change without parsing files ...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...es directly. When every manifest already records the requested version, the CLI i...
(QB_NEW_EN)
docs/roadmap.md
[grammar] ~127-~127: There might be a mistake here.
Context: ...n introduced alongside this feature. - Completion Criteria: A test case with ...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (13)
docs/roadmap.md (1)
89-95: Step 2.1 completion looks correctAccurately reflects implemented manifest version propagation.
lading/cli.py (1)
188-205: CLI wiring for bump(version, …) looks solidValidation then dispatch through _run_with_context to commands.bump.run is correct.
lading/commands/bump.py (2)
29-60: run(): version propagation and exclude handling LGTMWorkspace + root [package] updates, per-crate loop, and summary messaging behave as intended.
122-136: Atomic write implementation LGTMmkstemp in target dir + replace() + cleanup is correct and cross-platform-safe for same-filesystem replacements.
tests/bdd/features/cli.feature (1)
2-9: BDD scenarios align with the new versioned bump flowCovers happy path, invalid version, and no-change behavior; looks good.
Also applies to: 10-15, 16-22, 29-32
tests/unit/test_cli.py (5)
117-122: LGTM: dispatch wiring, argument capture, and output checks
- Correctly validates bump signature (workspace_root, version) and ensures config/workspace in kwargs.
- Publish path asserts expected positional args.
- Output assertions target the printed return values.
Also applies to: 142-169, 156-165, 167-169
199-206: LGTM: missing configuration error pathAsserts exit code and error message precisely when config is absent.
243-259: LGTM: invalid version short-circuitGood guard ensuring
bump.runis not called for invalid versions; error surfaced via exit code and stderr.
269-285: LGTM: direct Cyclopts invocation testValidates
cli.appinvocation path, propagating workspace and version tobump.run.
287-294: LGTM: workspace env context managerConfirms env var set/cleared correctly during context lifetime.
tests/bdd/steps/test_cli_steps.py (3)
167-174: LGTM: message assertion is resilientChecks for substrings rather than exact punctuation; robust to minor formatting changes.
176-186: LGTM: “no changes required” scenarioVerifies both the generic message and the “already ” detail.
224-232: LGTM: publish placeholder validationAsserts workspace path, crate count, and strip patch strategy in output.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
docs/lading-design.md(1 hunks)docs/usage-guide.md(3 hunks)lading/cli.py(3 hunks)lading/commands/bump.py(1 hunks)tests/bdd/features/cli.feature(2 hunks)tests/bdd/steps/test_cli_steps.py(7 hunks)tests/unit/test_bump_command.py(1 hunks)tests/unit/test_cli.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*.py: Python files must use snake_case filenames (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants must use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single leading underscore
Use typing everywhere and maintain full static type coverage
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only usage
Avoid Any; prefer precise types (TypeVar, Protocol, Literal, Union); use typing.cast only when necessary with justification; use object for unknown opaque values
Be explicit with return types (e.g., -> None, -> str) for all public functions and methods
Favor immutability: prefer tuples to lists and MappingProxyType for read-only mappings; document third-party frozendict if used
Use# pyright: ignoresparingly and include an explanation comment when used
Avoid side effects at import time (no global state mutation or actions on import)
Use docstrings (NumPy format) for public functions, classes, and modules
Explain tricky code with inline comments
**/*.py: Prefer context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual try/finally blocks
Use contextlib.contextmanager (@contextmanager) to implement simple, linear setup/teardown context managers
Implement a class with enter and exit for context managers that require internal state or more complex lifecycle handling
For file I/O, prefer with open(...) as ... over open()/try/finally/close patterns
Choose @contextmanager when control flow is linear and no persistent state is needed
Choose a class-based context manager when there is internal state, lifecycle methods, re-entry, or advanced context needs
**/*.py: Exception classes must end with the suffix 'Error' (N818)
Prefer specific built-ins (e.g., TypeError, ValueError) or domain exceptions over raising Exception directly (TRY003/TRY004)
Preserve causal chains...
Files:
tests/bdd/steps/test_cli_steps.pylading/commands/bump.pytests/unit/test_cli.pytests/unit/test_bump_command.pylading/cli.py
{**/unittests/test_*.py,tests/**/*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/**/*.py}: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective: test public behaviour, not internals
Files:
tests/bdd/steps/test_cli_steps.pytests/unit/test_cli.pytests/unit/test_bump_command.py
tests/**/*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, assert specific exception types (and optional message via regex) rather than using broad Exception (B017)
Files:
tests/bdd/steps/test_cli_steps.pytests/unit/test_cli.pytests/unit/test_bump_command.py
{README.md,docs/**}
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate documentation: keep README.md or a docs/ directory near reusable packages and include usage examples
Files:
docs/usage-guide.mddocs/lading-design.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use markdown files in docs/ as the knowledge base and source of truth for requirements, dependencies, and architectural decisions.
Proactively update relevant docs/ markdown when decisions, requirements, dependencies, or architecture change.
Files:
docs/usage-guide.mddocs/lading-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown files must pass markdownlint.
Markdown files containing Mermaid diagrams must pass nixie validation.
Files:
docs/usage-guide.mddocs/lading-design.md
🧬 Code graph analysis (5)
tests/bdd/steps/test_cli_steps.py (2)
tests/helpers/workspace_helpers.py (1)
install_cargo_stub(15-38)tests/conftest.py (1)
repo_root(16-18)
lading/commands/bump.py (4)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/config.py (2)
LadingConfig(82-103)current_configuration(155-161)lading/workspace/models.py (2)
WorkspaceGraph(43-52)load_workspace(55-62)lading/cli.py (1)
bump(192-207)
tests/unit/test_cli.py (6)
lading/cli.py (2)
main(118-153)publish(211-216)lading/config.py (1)
LadingConfig(82-103)tests/unit/test_bump_command.py (1)
_make_workspace(31-56)tests/unit/test_commands_placeholder.py (1)
_make_workspace(23-46)crate_tools/unittests/test_publish_workspace_dependencies.py (1)
workspace_root(16-21)lading/workspace/models.py (1)
WorkspaceGraph(43-52)
tests/unit/test_bump_command.py (4)
lading/cli.py (2)
bump(192-207)publish(211-216)lading/workspace/models.py (2)
WorkspaceCrate(30-40)WorkspaceGraph(43-52)lading/config.py (2)
LadingConfig(82-103)BumpConfig(36-53)lading/commands/bump.py (5)
run(30-60)_update_manifest(63-76)_select_table(85-101)_assign_version(104-118)_value_matches(121-125)
lading/cli.py (2)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/commands/bump.py (1)
run(30-60)
🪛 LanguageTool
docs/usage-guide.md
[grammar] ~67-~67: There might be a mistake here.
Context: ...rsions across the workspace. The command requires the target version as a positio...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ...onal argument and rejects inputs that do not match the <major>.<minor>.<patch> ...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ...semantic version pattern, while allowing optional pre-release and build metadata....
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...adata. All validation happens before the command loads workspace metadata, so mis...
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...validation, bump updates the workspace Cargo.toml and each member crate's man...
(QB_NEW_EN)
[grammar] ~74-~74: There might be a mistake here.
Context: ...nifest, unless the crate name appears in bump.exclude within lading.toml. ``...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...nifest(s).` This lets release automation assert the change without parsing files ...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ...es directly. When every manifest already records the requested version, the CLI i...
(QB_NEW_EN)
[grammar] ~129-~129: There might be a mistake here.
Context: ... using plumbum, and returns the parsed JSON mapping. Any execution errors or in...
(QB_NEW_EN)
docs/lading-design.md
[grammar] ~282-~282: There might be a mistake here.
Context: ...version value during the update pass. - The command reports a concise summary (...
(QB_NEW_EN)
[grammar] ~286-~286: There might be a mistake here.
Context: ...d" message instead of rewriting files. - Version arguments are validated at the C...
(QB_NEW_EN)
[grammar] ~289-~289: There might be a mistake here.
Context: ...optional pre-release and build metadata. - The legacy bump.doc_files configuratio...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
tests/bdd/features/cli.feature (1)
2-29: Scenarios match new CLI requirements.All revised scenarios explicitly pass the target version and assert the new outputs; they mirror the CLI changes accurately. Nice coverage of happy path, invalid input, exclusions, and no-op cases.
docs/usage-guide.md (1)
67-91: Documentation update looks great.The guide now clearly states the version argument requirement, validation rules, exclusion behavior, and output messages—exactly what users need post-change.
tests/unit/test_bump_command.py (1)
1-234: Excellent coverage in the new unit tests.Great job exercising the full bump pipeline—workspace/member updates, exclusions, normalization, lazy loading, idempotence, and trivia preservation. This suite gives strong confidence the tomlkit workflow behaves as intended.
docs/lading-design.md (1)
265-293: Design notes stay aligned with implementation.The updated Step 2.1 section accurately reflects the manifest rewrite strategy, exclusion handling, reporting, validation, and retirement of
bump.doc_files. Thanks for keeping the design doc in sync.lading/cli.py (1)
27-207: CLI validation and wiring are solid.The semantic-version regex, annotated parameter, early validation, and dispatcher updates are cleanly integrated—exactly what's needed for the new bump workflow.
1 participant
Summary
lading bumpCLI with a required version argument and add new unit and BDD coverageTesting
https://chatgpt.com/codex/tasks/task_e_68ed70ec06648322b760bf9786254695
Summary by Sourcery
Implement full version bumping in the
bumpsubcommand: accept a target version, update both workspace and crate Cargo.toml files using tomlkit, respect exclusions, and enhance documentation and tests to cover the new behavior.New Features:
bumpcommandbump.excludeconfiguration to skip specified cratesEnhancements:
bumprun functionDocumentation:
bumpcommand syntax and behaviorTests:
bump.run, covering version updates, exclusion logic, normalization, and fallback loadingSummary by CodeRabbit
New Features
Documentation
Tests