Implement crate packaging and publish with patch-stripping (tomlkit) - #40
Conversation
Reviewer's GuideIntroduce configurable patch-stripping into the publish workflow by loading and rewriting the staged Cargo.toml using TOMLKit, wiring in a new strip-patches strategy from configuration, standardizing execution logging, and covering the feature with extensive tests and updated documentation. Sequence diagram for applying patch-stripping during publish workflowsequenceDiagram
actor User
participant "lading publish"
participant "PublishPatchHelpers"
participant "Cargo.toml (staged)"
User->>"lading publish": Run publish command
"lading publish"->>"PublishPatchHelpers": Call _apply_strip_patch_strategy
"PublishPatchHelpers"->>"Cargo.toml (staged)": Load manifest
"PublishPatchHelpers"->>"Cargo.toml (staged)": Apply patch-stripping strategy
"PublishPatchHelpers"->>"Cargo.toml (staged)": Write modified manifest
"lading publish"->>User: Show publish plan and results
ER diagram for Cargo.toml patch table modificationerDiagram
CARGO_TOML {
string patch
string crates_io
}
PATCH_TABLE {
string crate_name
string patch_entry
string strategy
}
CARGO_TOML ||--o| PATCH_TABLE : contains
PATCH_TABLE }o--|| STRATEGY : uses
STRATEGY {
string type
string all
string per_crate
string false
}
Class diagram for patch-stripping helpers in publish.pyclassDiagram
class PublishPlan
class PublishPreparationError
class TOMLDocument
class StripPatchesSetting
class PublishPatchHelpers {
+_load_manifest_document(manifest_path: Path) TOMLDocument
+_write_manifest_document(manifest_path: Path, document: TOMLDocument)
+_get_patch_tables(document: TOMLDocument) tuple | None
+_strip_all_patch_entries(document: TOMLDocument) bool
+_strip_named_patch_entries(document: TOMLDocument, crate_names: Iterable[str]) bool
+_apply_strip_patch_strategy(staging_root: Path, plan: PublishPlan, strategy: StripPatchesSetting)
}
PublishPatchHelpers --> TOMLDocument
PublishPatchHelpers --> PublishPlan
PublishPatchHelpers --> StripPatchesSetting
PublishPatchHelpers --> PublishPreparationError
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. Note
|
| Cohort / File(s) | Summary |
|---|---|
Documentation docs/lading-design.md, docs/roadmap.md, docs/usage-guide.md |
Documented publish.strip_patches options ("all", "per-crate", false), explained that the staged temporary clone's Cargo.toml is edited in-place with tomlkit, described README propagation/symlink options, and marked the roadmap task complete. |
Publish command lading/commands/publish.py |
Added tomlkit-based manifest load/write helpers, manifest validation and PublishPreparationError mapping, helpers to locate/modify [patch] / [patch.crates-io], implemented _apply_strip_patch_strategy, wired strategy into run (now accepts options), and added related type aliases and re-exports. |
Publish diagnostics lading/commands/publish_diagnostics.py |
Adjusted stderr artifact regex (removed an unnecessary escape), changing artifact path matching semantics. |
BDD features & fixtures tests/bdd/features/cli.feature, tests/bdd/steps/config_fixtures.py, tests/bdd/steps/manifest_fixtures.py |
Added CLI scenarios for strip_patches strategies and fixtures to set publish.strip_patches and to populate [patch.crates-io] entries in workspace manifests. |
BDD step implementations tests/bdd/steps/test_publish_steps.py |
Added TOML parsing and staged-manifest inspection helpers/assertions for patch presence/omission and per-crate retention; adjusted preflight override parsing and plan assertions. |
Unit test utilities & tests tests/unit/publish/preflight_test_utils.py, tests/unit/publish/test_preflight_checks.py, tests/unit/publish/test_command_logging.py, tests/unit/publish/test_run_integration.py, tests/unit/test_publish_patch_strategy.py |
Changed preflight call extractor return shape; restored ORIGINAL_PREFLIGHT in several tests; updated logging capture logger name in some tests; updated integration tests to call run(..., options=PublishOptions(...)); added unit tests validating patch-stripping logic and edge cases. |
Sequence Diagram(s)
sequenceDiagram
participant CLI
participant PublishRun as publish.run
participant WorkspacePrep as prepare_workspace
participant StagingDir as staging_root
participant PatchStrip as _apply_strip_patch_strategy
participant TOML as tomlkit I/O
CLI->>PublishRun: run(..., options)
PublishRun->>WorkspacePrep: clone workspace to temp dir
WorkspacePrep-->>StagingDir: staging_root path
PublishRun->>PatchStrip: _apply_strip_patch_strategy(staging_root, plan, strategy)
alt strategy == "all"
PatchStrip->>TOML: load Cargo.toml
TOML-->>PatchStrip: TOMLDocument
PatchStrip->>PatchStrip: remove entire [patch.crates-io] table
PatchStrip->>TOML: write Cargo.toml
else strategy == "per-crate"
PatchStrip->>TOML: load Cargo.toml
TOML-->>PatchStrip: TOMLDocument
PatchStrip->>PatchStrip: remove entries for publishable crates only
PatchStrip->>PatchStrip: remove empty tables if needed
PatchStrip->>TOML: write Cargo.toml (if modified)
else strategy == false
PatchStrip-->>PatchStrip: no-op (leave manifest unchanged)
end
PatchStrip-->>PublishRun: return
PublishRun-->>CLI: print plan and staged location
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Areas needing extra attention:
- lading/commands/publish.py: TOML parsing/mutation, write-back, and PublishPreparationError handling.
- New tests: tests/unit/test_publish_patch_strategy.py and BDD scenarios for correctness and flakiness.
- Tests that change helper return shapes and restore ORIGINAL_PREFLIGHT.
Poem
🐇 I nibble toml with careful paws,
Per-crate or all — I follow laws.
In staged-clone burrows neat and round,
I strip or keep each patch I found.
Hooray — the publish plan hops safe to ground.
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Implement crate packaging and publish with patch-stripping (tomlkit)' clearly and specifically describes the main change: introducing publish workflow functionality with patch-stripping capabilities using tomlkit for TOML handling. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 91.84% which is sufficient. The required threshold is 80.00%. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
terragon/implement-publish-subcommand-kfw46u
📜 Recent 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 (1)
lading/commands/publish.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
lading/commands/publish.py
🧬 Code graph analysis (1)
lading/commands/publish.py (3)
lading/commands/publish_execution.py (5)
_CommandRunner(52-62)_invoke(81-94)_normalise_cmd_mox_command(185-195)_should_use_cmd_mox_stub(107-110)_split_command(97-104)lading/commands/publish_plan.py (5)
PublishPlan(22-34)plan_publication(146-195)PublishPlanError(17-18)_append_section(213-223)publishable_names(32-34)lading/utils/path.py (1)
normalise_workspace_root(10-16)
⏰ 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 (6)
lading/commands/publish.py (6)
51-65: Type alias wiring and manifest validation type look consistentThe re-export of
StripPatchesSettingand the_ManifestValidationtypealias cleanly express the configuration surface and the(document, (patch_table, crates_io)) | Nonecontract. This should keep the patch-stripping helpers easy to type-check and reuse.
331-357: Manifest read/parse/write helpers are robust and user-friendlyThe manifest I/O helpers correctly:
- Read with UTF‑8, wrap
FileNotFoundError,PermissionError, andOSErrorintoPublishPreparationErrorwith clear messages.- Parse TOML and convert parser failures into
PublishPreparationError.- Preserve TOML trivia/comments via
document.as_string()and enforce a final newline.- Handle write failures via
OSErrorand surface them asPublishPreparationError.This gives a solid, centralized error surface for all manifest-related issues; no changes needed.
360-370: Per-crate removal helper is correct and deterministic
_remove_per_crate_entriesdoes the right thing:
- Uses
dict.fromkeys(crate_names)to deduplicate while preserving the original ordering, which keeps updates deterministic.- Returns a
boolindicating whether anything was removed, which cleanly feeds into higher-level logic.No functional or style issues spotted here.
373-405: Patch-table resolution and manifest gating align with the configuration model
_resolve_patch_tablesand_validate_and_load_manifesttogether:
- Safely detect the presence of a
[patch.crates-io]mapping before attempting any mutations.- Short-circuit when
strategy is False, whenCargo.tomlis absent, or when the expected tables are missing/not mappings.- Return a precise
(document, (patch_table, crates_io))tuple only when patch-stripping is actually applicable.This matches the documented strategies (“all”, “per-crate”, and
falsefor disabled) and keeps non-patch or non-Cargo workspaces out of the strip path without raising spurious errors.
407-457: Patch cleanup and strategy application are well-factored and behave correctlyThe trio
_cleanup_empty_patch_tables,_apply_strategy_to_patches, and_apply_strip_patch_strategygives a clear flow:
"all"removes the entirecrates-iotable in one step."per-crate"uses_remove_per_crate_entriesto strip only publishable crates.- Unsupported non-
Falsestrategies raisePublishPreparationError, so misconfiguration never silently succeeds.- After a modification,
_cleanup_empty_patch_tablesprunes an emptycrates-ioand then the wholepatchtable when appropriate, preserving any other registries._apply_strip_patch_strategycentralizes validation, application, cleanup, and write-back, keeping complexity low and behavior easy to reason about.This structure should be straightforward to maintain and matches the behavior described in the PR notes and tests.
515-519: run() wiring for strip_patches is in the right place and uses staged manifestsHooking
_apply_strip_patch_strategyintorun()immediately afterprepare_workspaceensures:
- Only the staged
Cargo.tomlunderpreparation.staging_rootis mutated, never the original workspace.- The active configuration (
active_configuration.publish.strip_patches) fully drives behavior, consistent with how other publish options are handled.- The plan (and its
publishable_names) is already computed, so per-crate strategies have all necessary context.This integration point looks correct and matches the documented usage.
Comment @coderabbitai help to get the list of available commands and usage tips.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +355 to +374 def _strip_named_patch_entries(
document: TOMLDocument,
crate_names: cabc.Iterable[str],
) -> bool:
"""Remove patch entries that match ``crate_names``."""
patch_tables = _get_patch_tables(document)
if patch_tables is None:
return False
patch_table, crates_io = patch_tables
removed = False
for crate in dict.fromkeys(crate_names):
if crate in crates_io:
del crates_io[crate]
removed = True
if removed:
if not crates_io:
patch_table.pop("crates-io", None)
if not patch_table:
document.pop("patch", None)
return removed❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
leynos
changed the title
![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.
- Consider moving the TOML manifest manipulation and patch-stripping helpers into their own module to declutter publish.py and improve cohesion.
- There are several duplicate BDD test helpers for loading and inspecting staged manifests—extract them into a shared fixture to reduce repetition.
- The import aliasing of private functions from publish_plan and publish_execution is verbose—consider re-exporting or grouping them in those modules to simplify the import surface.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving the TOML manifest manipulation and patch-stripping helpers into their own module to declutter publish.py and improve cohesion.
- There are several duplicate BDD test helpers for loading and inspecting staged manifests—extract them into a shared fixture to reduce repetition.
- The import aliasing of private functions from publish_plan and publish_execution is verbose—consider re-exporting or grouping them in those modules to simplify the import surface.
## Individual Comments
### Comment 1
<location> `lading/commands/publish.py:310-314` </location>
<code_context>
return tuple(lines)
+def _load_manifest_document(manifest_path: Path) -> TOMLDocument:
+ """Parse and return the staged workspace manifest."""
+ try:
+ text = manifest_path.read_text(encoding="utf-8")
+ except FileNotFoundError as exc: # pragma: no cover - defensive guard
+ message = f"Workspace manifest not found at {manifest_path}"
+ raise PublishPreparationError(message) from exc
+ try:
+ return parse_toml(text)
+ except TOMLKitError as exc:
+ message = f"Failed to parse staged workspace manifest: {manifest_path}"
</code_context>
<issue_to_address>
**suggestion:** Consider handling generic I/O errors when reading the manifest.
Other exceptions such as PermissionError or OSError may also occur when reading the file. Consider catching these to ensure users receive clear error messages for all I/O issues.
```suggestion
try:
text = manifest_path.read_text(encoding="utf-8")
except FileNotFoundError as exc: # pragma: no cover - defensive guard
message = f"Workspace manifest not found at {manifest_path}"
raise PublishPreparationError(message) from exc
except (PermissionError, OSError) as exc: # pragma: no cover - defensive guard
message = f"Unable to read workspace manifest at {manifest_path}: {exc}"
raise PublishPreparationError(message) from exc
```
</issue_to_address>
### Comment 2
<location> `lading/commands/publish.py:322-327` </location>
<code_context>
+ raise PublishPreparationError(message) from exc
+
+
+def _write_manifest_document(manifest_path: Path, document: TOMLDocument) -> None:
+ """Persist ``document`` back to ``manifest_path`` preserving trivia."""
+ text = document.as_string()
+ if not text.endswith("\n"):
+ text = f"{text}\n"
+ manifest_path.write_text(text, encoding="utf-8")
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Add error handling for manifest write failures.
Currently, write failures to the manifest file are not handled specifically. Please catch exceptions during file write and raise a more informative error or handle them to improve user feedback.
```suggestion
def _write_manifest_document(manifest_path: Path, document: TOMLDocument) -> None:
"""Persist ``document`` back to ``manifest_path`` preserving trivia."""
text = document.as_string()
if not text.endswith("\n"):
text = f"{text}\n"
try:
manifest_path.write_text(text, encoding="utf-8")
except (OSError, IOError) as exc:
message = f"Failed to write manifest to {manifest_path}: {exc}"
raise PublishPreparationError(message) from exc
```
</issue_to_address>
### Comment 3
<location> `tests/unit/test_publish_patch_strategy.py:77-101` </location>
<code_context>
+ assert "patch" not in document
+
+
+def test_strip_patches_per_crate_removes_publishable_only(
+ tmp_path: Path,
+ make_plan_factory: typ.Callable[[Path, tuple[str, ...]], publish.PublishPlan],
+) -> None:
+ """Strategy 'per-crate' removes only entries for publishable crates."""
+ workspace_root = tmp_path / "workspace"
+ workspace_root.mkdir()
+ manifest_text = _base_manifest(
+ "[patch.crates-io]\n"
+ 'alpha = { path = "crates/alpha" }\n'
+ 'serde = { git = "https://example.com/serde" }\n'
+ )
+ _write_manifest(workspace_root, manifest_text)
+ plan = make_plan_factory(workspace_root, ("alpha",))
+
+ publish._apply_strip_patch_strategy(workspace_root, plan, "per-crate")
+
+ document = parse_toml((workspace_root / "Cargo.toml").read_text(encoding="utf-8"))
+ patch_table = document.get("patch", {})
+ crates_io = patch_table.get("crates-io", {})
+ assert "alpha" not in crates_io
+ assert "serde" in crates_io
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Good test for 'per-crate' strategy, but missing edge case for empty patch table.
Add a test where all patch entries are removed to confirm that empty [patch] and [patch.crates-io] sections are deleted, ensuring cleanup logic works as intended.
```suggestion
def test_strip_patches_per_crate_removes_publishable_only(
tmp_path: Path,
make_plan_factory: typ.Callable[[Path, tuple[str, ...]], publish.PublishPlan],
) -> None:
"""Strategy 'per-crate' removes only entries for publishable crates."""
workspace_root = tmp_path / "workspace"
workspace_root.mkdir()
manifest_text = _base_manifest(
"[patch.crates-io]\n"
'alpha = { path = "crates/alpha" }\n'
'serde = { git = "https://example.com/serde" }\n'
)
_write_manifest(workspace_root, manifest_text)
plan = make_plan_factory(workspace_root, ("alpha",))
publish._apply_strip_patch_strategy(workspace_root, plan, "per-crate")
document = parse_toml((workspace_root / "Cargo.toml").read_text(encoding="utf-8"))
patch_table = document.get("patch", {})
crates_io = patch_table.get("crates-io", {})
assert "alpha" not in crates_io
assert "serde" in crates_io
def test_strip_patches_per_crate_removes_entire_patch_table_when_empty(
tmp_path: Path,
make_plan_factory: typ.Callable[[Path, tuple[str, ...]], publish.PublishPlan],
) -> None:
"""Strategy 'per-crate' removes all patch entries and cleans up empty patch tables."""
workspace_root = tmp_path / "workspace"
workspace_root.mkdir()
manifest_text = _base_manifest(
"[patch.crates-io]\n"
'alpha = { path = "crates/alpha" }\n'
'beta = { path = "crates/beta" }\n'
)
_write_manifest(workspace_root, manifest_text)
plan = make_plan_factory(workspace_root, ("alpha", "beta"))
publish._apply_strip_patch_strategy(workspace_root, plan, "per-crate")
document = parse_toml((workspace_root / "Cargo.toml").read_text(encoding="utf-8"))
# Both patch and patch.crates-io should be removed
assert "patch" not in document
```
</issue_to_address>
### Comment 4
<location> `lading/commands/publish.py:386` </location>
<code_context>
+ return removed
+
+
+def _apply_strip_patch_strategy(
+ staging_root: Path,
+ plan: PublishPlan,
</code_context>
<issue_to_address>
**issue (complexity):** Consider collapsing multiple patch-stripping helpers into a single loop and removing unnecessary module re-exports to simplify the code.
```suggestion
# 1) Collapse the 4 helpers + `_get_patch_tables`/`_cleanup_empty_patch_tables`
# into one straightforward loop in `_apply_strip_patch_strategy`
#
# BEFORE (new helpers scattered above):
# modified = _strip_all_patch_entries(document) # or
# modified = _strip_named_patch_entries(document, plan.publishable_names)
# if modified:
# _write_manifest_document(manifest_path, document)
#
# AFTER (inline in one pass—same behavior, fewer symbols):
def _apply_strip_patch_strategy(
staging_root: Path,
plan: PublishPlan,
strategy: StripPatchesSetting,
) -> None:
if not strategy:
return
manifest_path = staging_root / "Cargo.toml"
if not manifest_path.exists():
return
doc = _load_manifest_document(manifest_path)
patch = doc.get("patch")
if not isinstance(patch, dict):
return
crates = patch.get("crates-io")
if not isinstance(crates, dict):
return
removed = False
if strategy == "all":
removed = patch.pop("crates-io", None) is not None
else: # per-crate
for name in plan.publishable_names:
if crates.pop(name, None) is not None:
removed = True
if not removed:
return
# cleanup empty tables
if not crates:
patch.pop("crates-io", None)
if not patch:
doc.pop("patch", None)
_write_manifest_document(manifest_path, doc)
# Then you can safely delete:
# _get_patch_tables
# _strip_all_patch_entries
# _strip_named_patch_entries
# _cleanup_empty_patch_tables
# 2) Remove the module aliases + re-exports noise and import only what’s used
#
# BEFORE:
# from lading.commands import publish_execution as _publish_execution
# _CommandRunner = _publish_execution._CommandRunner
# _invoke = _publish_execution._invoke
# ...
#
# AFTER:
from lading.commands.publish_execution import (
_CommandRunner,
_invoke,
_split_command,
_normalise_cmd_mox_command,
_should_use_cmd_mox_stub,
)
from lading.commands.publish_plan import (
PublishPlan,
PublishPlanError,
_append_section,
_format_plan,
plan_publication,
)
```
</issue_to_address>
### Comment 5
<location> `lading/commands/publish.py:308` </location>
<code_context>
return tuple(lines)
+def _load_manifest_document(manifest_path: Path) -> TOMLDocument:
+ """Parse and return the staged workspace manifest."""
+ try:
</code_context>
<issue_to_address>
**issue (review_instructions):** You must add both behavioural and unit tests for the new patch-stripping feature.
The new functions for patch-stripping logic (_load_manifest_document, _write_manifest_document, _get_patch_tables, _strip_all_patch_entries, _cleanup_empty_patch_tables, _strip_named_patch_entries, _apply_strip_patch_strategy) implement a new feature. Ensure there are both unit and behavioural (integration) tests covering all code paths, including error handling and edge cases.
<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> `tests/bdd/steps/test_publish_steps.py:326-334` </location>
<code_context>
@given(
parsers.re(
r'the preflight command "(?P<command>.+)" exits with '
r'code (?P<exit_code>\d+) and stderr "(?P<stderr>.*)"'
)
)
def given_preflight_command_override(
preflight_overrides: dict[tuple[str, ...], _CommandResponse],
command: str,
exit_code: str,
stderr: str,
) -> None:
"""Override an arbitrary pre-flight command with a custom result."""
exit_code_int = int(exit_code)
tokens = tuple(segment for segment in command.split() if segment)
if not tokens:
message = "preflight command override requires tokens"
raise AssertionError(message)
preflight_overrides[tokens] = _CommandResponse(
exit_code=exit_code_int,
stderr=stderr,
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Move assignments closer to their usage ([`move-assign`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/move-assign/))
- Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- 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/))
- Inline variable that is only used once ([`inline-variable`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/inline-variable/))
```suggestion
if tokens := tuple(segment for segment in command.split() if segment):
exit_code_int = int(exit_code)
preflight_overrides[tokens] = _CommandResponse(
exit_code=exit_code_int,
stderr=stderr,
)
else:
raise AssertionError("preflight command override requires tokens")
```
</issue_to_address>
### Comment 7
<location> `tests/bdd/steps/test_publish_steps.py:365-367` </location>
<code_context>
def _get_patch_entries(document: typ.Mapping[str, typ.Any]) -> dict[str, typ.Any]:
"""Return the ``[patch.crates-io]`` mapping if it exists."""
patch_table = document.get("patch")
if not isinstance(patch_table, typ.Mapping):
return {}
crates_io = patch_table.get("crates-io")
if not isinstance(crates_io, typ.Mapping):
return {}
return dict(crates_io)
</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/))
- Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
```suggestion
return {} if not isinstance(crates_io, typ.Mapping) else dict(crates_io)
```
</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 (1)
lading/commands/publish_execution.py (1)
15-17: Consider preserving__name__for the logger.Hardcoding the logger name to
"lading.commands.publish"deviates from the standard patternlogging.getLogger(__name__)recommended in the coding guidelines (LOG015). While the comment explains the rationale for test observability, this approach couples the module to test requirements.Alternative: Configure test logging to capture logs from the actual module name (
lading.commands.publish_execution), which would maintain the standard logging pattern while still providing test visibility.As per coding guidelines.
📜 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 (14)
docs/lading-design.md(1 hunks)docs/roadmap.md(1 hunks)docs/usage-guide.md(1 hunks)lading/commands/publish.py(3 hunks)lading/commands/publish_diagnostics.py(1 hunks)lading/commands/publish_execution.py(1 hunks)tests/bdd/features/cli.feature(1 hunks)tests/bdd/steps/config_fixtures.py(2 hunks)tests/bdd/steps/manifest_fixtures.py(2 hunks)tests/bdd/steps/test_publish_steps.py(5 hunks)tests/unit/publish/preflight_test_utils.py(1 hunks)tests/unit/publish/test_preflight_checks.py(5 hunks)tests/unit/publish/test_run_integration.py(1 hunks)tests/unit/test_publish_patch_strategy.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
{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/roadmap.mddocs/lading-design.mddocs/usage-guide.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use docs/ markdown as the knowledge base and source of truth for requirements, dependencies, and architecture
Proactively update relevant docs/ markdown when decisions, requirements, dependencies, or architecture change
Files:
docs/roadmap.mddocs/lading-design.mddocs/usage-guide.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Markdown quality gates: .md files must pass markdownlint (make markdownlint) and Mermaid validation via nixie (make nixie) before commit
Files:
docs/roadmap.mddocs/lading-design.mddocs/usage-guide.md
**/*.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/publish/preflight_test_utils.pylading/commands/publish.pytests/unit/publish/test_preflight_checks.pytests/bdd/steps/manifest_fixtures.pytests/unit/test_publish_patch_strategy.pylading/commands/publish_execution.pytests/bdd/steps/test_publish_steps.pylading/commands/publish_diagnostics.pytests/bdd/steps/config_fixtures.pytests/unit/publish/test_run_integration.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/publish/preflight_test_utils.pytests/unit/publish/test_preflight_checks.pytests/bdd/steps/manifest_fixtures.pytests/unit/test_publish_patch_strategy.pytests/bdd/steps/test_publish_steps.pytests/bdd/steps/config_fixtures.pytests/unit/publish/test_run_integration.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/publish/preflight_test_utils.pytests/unit/publish/test_preflight_checks.pytests/bdd/steps/manifest_fixtures.pytests/unit/test_publish_patch_strategy.pytests/bdd/steps/test_publish_steps.pytests/bdd/steps/config_fixtures.pytests/unit/publish/test_run_integration.py
🧬 Code graph analysis (5)
lading/commands/publish.py (4)
lading/commands/publish_diagnostics.py (1)
_append_compiletest_diagnostics(56-79)lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/commands/publish_plan.py (5)
PublishPlan(22-34)_append_section(213-223)_format_plan(226-257)plan_publication(146-195)publishable_names(32-34)lading/commands/publish_execution.py (5)
_CommandRunner(25-35)_invoke(45-74)_split_command(77-84)_normalise_cmd_mox_command(132-142)_should_use_cmd_mox_stub(87-90)
tests/unit/test_publish_patch_strategy.py (3)
lading/workspace/models.py (1)
WorkspaceCrate(59-69)lading/commands/publish_plan.py (2)
PublishPlan(22-34)publishable_names(32-34)lading/commands/publish.py (1)
_apply_strip_patch_strategy(386-406)
tests/bdd/steps/test_publish_steps.py (2)
tests/bdd/steps/test_common_steps.py (1)
_run_cli(24-49)tests/unit/conftest.py (1)
staging_root(184-186)
tests/bdd/steps/config_fixtures.py (1)
tests/bdd/toml_utils.py (2)
load_or_create_document(25-29)ensure_table(32-38)
tests/unit/publish/test_run_integration.py (2)
lading/cli.py (1)
publish(284-299)lading/commands/publish.py (2)
run(438-473)PublishOptions(65-98)
⏰ 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). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (24)
docs/roadmap.md (1)
190-190: LGTM!The roadmap update correctly marks the "Implement Configurable Patch Stripping" feature as completed, aligning with the implemented functionality described in the PR.
tests/bdd/steps/manifest_fixtures.py (2)
8-8: LGTM!The imported
inline_tableandtableare correctly used in the new fixture function to construct TOML structures.
79-103: LGTM!The fixture correctly constructs
[patch.crates-io]entries for test scenarios. The hardcoded path pattern../{name}is appropriate for BDD test fixtures where consistent directory layouts are expected.tests/bdd/steps/config_fixtures.py (2)
34-41: LGTM!The helper function correctly uses the TOML utilities to set the
publish.strip_patchesconfiguration value. Theobjecttype for thevalueparameter appropriately accommodates both string strategies and boolean values.
106-119: LGTM!Both fixtures correctly delegate to the helper function and follow BDD patterns. The separation between string strategy and boolean false values is clear and appropriate for different test scenarios.
tests/unit/publish/preflight_test_utils.py (1)
52-61: LGTM!The simplified return type improves clarity by excluding the unused environment mapping. The underscore prefix on
_env(line 56) correctly signals the intentional discard of that value.docs/lading-design.md (1)
175-183: LGTM!The implementation detail clearly documents how the patch-stripping feature works, including the preservation of TOML formatting via
tomlkit, the three supported strategies, and error handling. This provides valuable context for maintainers.tests/unit/publish/test_preflight_checks.py (1)
295-295: LGTM!The monkeypatch ensures this test and the other updated tests (lines 333, 373, 409, 460) exercise the actual preflight implementation rather than any mocked or stubbed version. This improves test reliability for preflight-specific behavior.
lading/commands/publish_diagnostics.py (1)
8-8: No changes needed—current code is correct.The regex pattern in the current code shows
\.stderr)with the escaped dot, which is the correct and more precise form. The test output confirms: the escaped dot pattern correctly rejects false matches like/path/to/filexstderr, while an unescaped dot would incorrectly match them.The review comment's warning assumes a problematic change occurred, but the code as shown retains the correct escaped-dot pattern. There is no issue to address.
Likely an incorrect or invalid review comment.
tests/unit/publish/test_run_integration.py (1)
231-236: Correctly exercising forbid-dirty preflight path via optionsPassing
options=publish.PublishOptions(allow_dirty=False)keeps this test aligned with the newrunsignature and explicitly drives the git-cleanliness path in_run_preflight_checks. The assertions aboutcwdand recorded commands remain valid.docs/usage-guide.md (1)
232-244: Strip-patches documentation aligns with implementationThe new section clearly explains
"all","per-crate", andfalsestrategies and correctly notes that only the stagedCargo.tomlin the temporary clone is mutated. This matches the_apply_strip_patch_strategysemantics.tests/bdd/features/cli.feature (1)
234-261: Good end-to-end coverage of strip-patches modesThese three scenarios cleanly exercise
"all","per-crate", andfalsebehaviours against a workspace with patches foralphaandserde, and verify the staged manifest via dedicated steps. This gives solid BDD coverage for the new configuration surface.lading/commands/publish.py (6)
6-7: Imports, type shims, and re-exports are structured and backwards-compatibleIntroducing
collections.abc as cabc, tomlkit parsing, theTOMLDocumenttyping shim, and the alias exports (PublishPlan,_CommandRunner,_invoke, etc.) keeps this module typed while avoiding runtime ImportError issues and preserving the previouslading.commands.publishsurface that tests and callers rely on. Themetadata_modulealias also preserves the prior API without adding side effects.Also applies to: 14-21, 23-42
308-320: TOML manifest load/write helpers handle errors and trivia correctly
_load_manifest_documentwraps filesystem andtomlkiterrors inPublishPreparationError, giving callers a clean, domain-specific failure mode._write_manifest_documentusesdocument.as_string()and enforces a trailing newline, which is appropriate for preserving tomlkit trivia and formatting. No issues here.Also applies to: 322-328
330-341: Patch-table discovery and “all” stripping logic look robust
_get_patch_tablesdefensively verifies bothpatchandcrates-iotables are mutable mappings before proceeding, which is important when dealing with hand-edited manifests._strip_all_patch_entriesdrops thecrates-iotable and removespatchonly when it becomes empty, matching the documented semantics of removing the[patch.crates-io]section without touching other registries.Also applies to: 343-353
355-365: Refactored per-crate stripping keeps behaviour and reduces nestingExtracting
_cleanup_empty_patch_tablesand calling it from_strip_named_patch_entriesonceremovedis true keeps the function’s control flow shallow while preserving semantics:
- Only
[patch.crates-io]entries for the given crate names are removed.dict.fromkeys(crate_names)de-duplicates while preserving order.- Empty
crates-ioand then emptypatchtables are cleaned up via the helper.This aligns with the Codescene feedback without introducing behavioural changes.
Also applies to: 367-383
386-407: Strategy dispatcher cleanly separates modes and guards unsupported values
_apply_strip_patch_strategy:
- Treats
strategy is Falseas a no-op, matching the configuration intent.- Safely exits when
Cargo.tomlis missing in the staging root.- Dispatches
"all"vs"per-crate"to the appropriate helpers and raisesPublishPreparationErrorfor unexpected values (backstopped by config validation).- Only writes the manifest back when a modification actually occurred.
Behaviour matches the tests in
tests/unit/test_publish_patch_strategy.pyand the usage-guide description.
438-452: run() options wiring and patch-stripping integration are coherent
run()now:
- Normalises the workspace root and merges explicit arguments with
PublishOptions(including configuration/workspace overrides and a customcommand_runner).- Runs preflight checks with
allow_dirtyfromeffective_options, matching the CLI--forbid-dirtybehaviour.- Calls
prepare_workspacewith the originaloptionsobject so staging respects build-directory, symlink, and cleanup settings.- Applies the configured
publish.strip_patchesstrategy to the staged manifest and includes the same value in the formatted plan summary.The sequence and parameter choices are consistent and match how tests and docs expect the flow to behave.
Also applies to: 460-471, 463-468
tests/unit/test_publish_patch_strategy.py (1)
1-119: Unit tests thoroughly exercise strip-patches behaviourThe
make_plan_factoryfixture,_write_manifest/_base_manifesthelpers, and the three tests together validate:
"all"removes[patch.crates-io]entirely."per-crate"removes only publishable crate entries (herealpha) and preserves others (serde).strategy=Falseis a true no-op.Using tomlkit to parse the post-stripping manifest keeps these assertions close to how the production code manipulates TOML. Types are guarded under
TYPE_CHECKINGwithfrom __future__ import annotations, which fits the typing guidelines.tests/bdd/steps/test_publish_steps.py (5)
12-13: tomlkit integration and TOMLDocument typing shim are appropriate for testsImporting
parse_tomland introducing aTOMLDocumentalias guarded byTYPE_CHECKING(with a runtimeAnyfallback) lets these steps parse and type-annotate staged manifests without adding runtime coupling or risking ImportError in type-checking environments. This matches the pattern used in the main publish module.Also applies to: 27-33
156-165: Preflight expectation normalisation matches cargo:: stub scheme
_resolve_preflight_expectationnow directly maps any("cargo", <subcmd>, ...)tuple to("cargo::<subcmd>", remaining_args), which aligns with_normalise_cmd_mox_commandinpublish_execution. This keeps cmd-mox stub labels consistent across check/test and auxiliary cargo commands.
313-333: Regex-based preflight override and exit-code parsing improve robustnessSwitching to a regex parser for
given_preflight_command_overrideand acceptingexit_codeas a string (then converting viaint) removes implicit type assumptions from pytest-bdd and ensures only numeric exit codes match the step. The explicit empty-token check and assertion message remain helpful for debugging mis-specified overrides.
337-345: Plan header assertion is resilient to strategy value changesChanging
then_publish_prints_planto assertlines[1].startswith("Strip patch strategy:")decouples the test from any specific default strategy value while still verifying that the plan includes the configured strip-patches mode. This is more future-proof.
348-404: Staged-manifest inspection helpers correctly validate patch entriesThe new helpers:
_load_staged_manifestderives the staging root from the CLI output and loadsCargo.tomlvia tomlkit, failing fast with a clear AssertionError if missing._get_patch_entriessafely extracts[patch.crates-io]as a plain dict, returning{}when the section or table is absent or of the wrong type._split_namesnormalises comma-separated crate lists.- The three
then_…steps assert that the staged manifest has no patch entries at all, omits specific entries, or retains specific entries.Together they give strong end-to-end coverage of the strip-patches behaviour at the CLI level and align with the unit tests and implementation.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +375 to +408 def _apply_strip_patch_strategy(
staging_root: Path,
plan: PublishPlan,
strategy: StripPatchesSetting,
) -> None:
"""Modify the staged manifest according to ``publish.strip_patches``."""
if strategy is False:
return
manifest_path = staging_root / "Cargo.toml"
if not manifest_path.exists():
return
document = _load_manifest_document(manifest_path)
patch_tables = _resolve_patch_tables(document)
if patch_tables is None:
return
patch_table, crates_io = patch_tables
modified = False
if strategy == "all":
modified = patch_table.pop("crates-io", None) is not None
elif strategy == "per-crate":
modified = _remove_per_crate_entries(crates_io, plan.publishable_names)
else: # pragma: no cover - guarded by configuration validation
message = f"Unsupported strip patch strategy: {strategy}"
raise PublishPreparationError(message)
if not modified:
return
if not crates_io:
patch_table.pop("crates-io", None)
if not patch_table:
document.pop("patch", None)
_write_manifest_document(manifest_path, document)❌ New issue: Complex Method |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/unit/test_publish_patch_strategy.py Comment on lines +123 to +140 def test_strip_patches_disabled_keeps_section(
tmp_path: Path,
make_plan_factory: typ.Callable[[Path, tuple[str, ...]], publish.PublishPlan],
) -> None:
"""Boolean false leaves the patch section untouched."""
workspace_root = tmp_path / "workspace"
workspace_root.mkdir()
manifest_text = _base_manifest(
'[patch.crates-io]\nalpha = { path = "crates/alpha" }\n'
)
_write_manifest(workspace_root, manifest_text)
plan = make_plan_factory(workspace_root, ("alpha",))
publish._apply_strip_patch_strategy(workspace_root, plan, strategy=False)
document = parse_toml((workspace_root / "Cargo.toml").read_text(encoding="utf-8"))
patch_table = document.get("patch", {})
assert "crates-io" in patch_table❌ New issue: Code Duplication |
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Add a publish.strip_patches setting to control removal of [patch.crates-io] entries in the staged workspace Cargo.toml during the publish command. - "all" removes the entire [patch.crates-io] section. - "per-crate" removes patch entries for publishable crates only. - false leaves patch entries untouched. This preserves or removes patch overrides based on configuration to ensure correct dependency resolution during package publication. Includes comprehensive BDD and unit tests verifying behavior and updates documentation to explain patch stripping configuration and staging manifest normalization. Closes: #<issue_if_any> Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Extract cleanup logic for empty patch tables in publish.py into a dedicated _helper function _cleanup_empty_patch_tables to improve code clarity and reduce duplication in _strip_named_patch_entries. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
…andling - Consolidate patch entry removals with clearer helper functions - Add error handling for manifest file read/write operations - Clean up unused and duplicated code in patch section management - Adjust logger usage in publish_execution module - Enhance tests for patch stripping strategies and patch table cleanup - Minor improvements for code clarity and maintainability in publish commands Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
17d0297 to
7f8f59f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/bdd/steps/test_publish_steps.py (1)
156-164: Restore the restrictive cargo subcommand check.The condition at line 162 was incorrectly relaxed. The publish module's preflight checks are restricted to
cargo checkandcargo test(confirmed by the function signature at line 613 oflading/commands/publish.py). The condition should be restored toif program == "cargo" and argument_tuple and argument_tuple[0] in {"check", "test"}to match actual behavior and prevent accepting arbitrary cargo subcommands via config overrides.
🧹 Nitpick comments (1)
tests/unit/test_publish_patch_strategy.py (1)
102-140: Good coverage of edge cases and disabled strategy.The tests properly validate cleanup of empty patch tables and the disabled (
False) strategy behavior.Minor: Line 136 uses
strategy=False(keyword argument) while the other tests use positional arguments. Consider using positional arguments consistently across all tests for uniformity.
📜 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 (4)
lading/commands/publish.py(3 hunks)tests/bdd/steps/test_publish_steps.py(5 hunks)tests/unit/publish/test_command_logging.py(2 hunks)tests/unit/test_publish_patch_strategy.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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_publish_patch_strategy.pytests/bdd/steps/test_publish_steps.pytests/unit/publish/test_command_logging.pylading/commands/publish.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_publish_patch_strategy.pytests/bdd/steps/test_publish_steps.pytests/unit/publish/test_command_logging.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_publish_patch_strategy.pytests/bdd/steps/test_publish_steps.pytests/unit/publish/test_command_logging.py
🧬 Code graph analysis (3)
tests/unit/test_publish_patch_strategy.py (3)
lading/workspace/models.py (1)
WorkspaceCrate(59-69)lading/commands/publish_plan.py (2)
PublishPlan(22-34)publishable_names(32-34)lading/commands/publish.py (1)
_apply_strip_patch_strategy(375-408)
tests/bdd/steps/test_publish_steps.py (2)
tests/bdd/steps/test_common_steps.py (1)
_run_cli(24-49)tests/unit/conftest.py (1)
staging_root(184-186)
lading/commands/publish.py (2)
lading/commands/publish_execution.py (5)
_CommandRunner(23-33)_invoke(43-72)_normalise_cmd_mox_command(130-140)_should_use_cmd_mox_stub(85-88)_split_command(75-82)lading/commands/publish_plan.py (5)
PublishPlan(22-34)_format_plan(226-257)PublishPlanError(17-18)_append_section(213-223)publishable_names(32-34)
⏰ 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 (14)
tests/unit/publish/test_command_logging.py (1)
25-25: LGTM! Logger name correctly aligned with module structure.The logger name change from
lading.commands.publishtolading.commands.publish_executioncorrectly reflects where_invokeis now implemented.Also applies to: 37-37
tests/unit/test_publish_patch_strategy.py (3)
18-56: LGTM! Well-structured test fixtures and helpers.The
make_plan_factoryfixture and helper functions provide clean, focused utilities for building test scenarios.
59-75: LGTM! Test validates "all" strategy correctly.The test confirms that the "all" strategy removes the entire
[patch]section as expected.
78-99: LGTM! Test validates selective "per-crate" removal.The test correctly verifies that only publishable crates are removed while non-publishable entries are retained.
lading/commands/publish.py (6)
6-6: LGTM! Imports follow coding guidelines.The
collections.abc as cabcalias and theTYPE_CHECKINGguard forTOMLDocumentwith a runtime fallback correctly follow the typing best practices.Also applies to: 14-21
321-335: LGTM! Comprehensive error handling for manifest loading.The function properly handles file access errors (
FileNotFoundError,PermissionError,OSError) and parsing errors (TOMLKitError), providing clear context in each case.
338-347: LGTM! Proper error handling for manifest writes.The function ensures a trailing newline and handles write failures with clear error messages.
350-372: LGTM! Clean helpers with good defensive programming.
_remove_per_crate_entriesusesdict.fromkeysfor deduplication, and_resolve_patch_tablesvalidates types before returning patch tables. Both functions are well-structured.
375-408: LGTM! Well-structured patch-stripping implementation.The function uses appropriate early returns to reduce nesting and clearly implements the three strategies (False, "all", "per-crate"). Cleanup logic properly removes empty patch tables.
466-470: LGTM! Proper integration of patch-stripping into the publish workflow.The patch-stripping step is correctly positioned after workspace preparation and uses the configured strategy from
active_configuration.publish.strip_patches.tests/bdd/steps/test_publish_steps.py (4)
12-12: LGTM! Imports follow typing best practices.The
tomlkitimports andTYPE_CHECKINGguard forTOMLDocumentalign with the patterns established in the implementation files.Also applies to: 28-32
314-333: LGTM! Improved preflight command override parsing.The switch to
parsers.rewith a regex pattern and the walrus operator usage make the parsing cleaner and more robust. Theexit_codeparameter type change frominttostr(converted internally) is correct for pytest-bdd parameter extraction.
343-343: LGTM! Flexible assertion for strip patch strategy.Changing to
startswithallows the test to work with all strategy values ("all", "per-crate",False) while still validating that the strategy is reported in the plan.
347-401: LGTM! Well-structured helpers for staged manifest inspection.The new helper functions provide clean, focused utilities for loading and inspecting the staged manifest's patch entries. The defensive type checking in
_get_patch_entriesand clear naming make these easy to understand and use.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Complex Methodlading/commands/publish.py: _apply_strip_patch_strategy What lead to degradation?_apply_strip_patch_strategy has a cyclomatic complexity of 11, threshold = 9 Why does this problem occur?A Complex Method has a high cyclomatic complexity. The recommended threshold for the Python language is a cyclomatic complexity lower than 9. How to fix it?There are many reasons for Complex Method. Sometimes, another design approach is beneficial such as a) modeling state using an explicit state machine rather than conditionals, or b) using table lookup rather than long chains of logic. In other scenarios, the function can be split using EXTRACT FUNCTION. Just make sure you extract natural and cohesive functions. Complex Methods can also be addressed by identifying complex conditional expressions and then using the DECOMPOSE CONDITIONAL refactoring. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# complex_method.js
function postItem(item) {
if (!item.id) {
- if (item.x != null && item.y != null) {
- post(item);
- } else {
- throw Error("Item must have x and y");
- }
+ // extract a separate function for creating new item
+ postNew(item);
} else {
- if (item.x < 10 && item.y > 25) {
- put(item);
- } else {
- throw Error("Item must have an x and y value between 10 and 25");
- }
+ // and one for updating existing items
+ updateItem(item);
}
}
+
+function postNew(item) {
+ validateNew(item);
+ post(item);
+}
+
+function updateItem(item) {
+ validateUpdate(item);
+ put(item);
+}
+ |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationtests/unit/test_publish_patch_strategy.py: What lead to degradation?The module contains 3 functions with similar structure: test_strip_patches_all_removes_patch_section,test_strip_patches_disabled_keeps_section,test_strip_patches_per_crate_removes_entire_table_when_empty Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
Tip For best results, initiate chat on the files or code changes. @leynos I've already provided a comprehensive fix for this issue earlier in this thread. You can see my previous response where I suggested extracting three helper functions ( If that approach doesn't suit your needs or you'd like an alternative refactoring strategy, I can suggest a different approach. For example: Alternative Approach: Strategy Pattern with Table Lookup Instead of extracting helper functions, we could use a strategy pattern with a lookup table to eliminate the branching logic: def _strip_all_patches(
patch_table: cabc.MutableMapping[str, typ.Any],
crates_io: cabc.MutableMapping[str, typ.Any],
publishable_names: tuple[str, ...],
) -> bool:
"""Remove all patch entries."""
return patch_table.pop("crates-io", None) is not None
def _strip_per_crate_patches(
patch_table: cabc.MutableMapping[str, typ.Any],
crates_io: cabc.MutableMapping[str, typ.Any],
publishable_names: tuple[str, ...],
) -> bool:
"""Remove only publishable crate patch entries."""
return _remove_per_crate_entries(crates_io, publishable_names)
# Strategy lookup table
_STRIP_STRATEGIES = {
"all": _strip_all_patches,
"per-crate": _strip_per_crate_patches,
}
def _apply_strip_patch_strategy(
staging_root: Path,
plan: PublishPlan,
strategy: StripPatchesSetting,
) -> None:
"""Modify the staged manifest according to ``publish.strip_patches``."""
if strategy is False:
return
manifest_path = staging_root / "Cargo.toml"
if not manifest_path.exists():
return
document = _load_manifest_document(manifest_path)
patch_tables = _resolve_patch_tables(document)
if patch_tables is None:
return
patch_table, crates_io = patch_tables
# Use strategy lookup instead of branching
strategy_fn = _STRIP_STRATEGIES.get(strategy)
if strategy_fn is None: # pragma: no cover
message = f"Unsupported strip patch strategy: {strategy}"
raise PublishPreparationError(message)
modified = strategy_fn(patch_table, crates_io, plan.publishable_names)
if not modified:
return
if not crates_io:
patch_table.pop("crates-io", None)
if not patch_table:
document.pop("patch", None)
_write_manifest_document(manifest_path, document)Would you like me to provide the AI coding agent prompt for this alternative approach, or would you prefer to proceed with the first solution I suggested earlier? |
|
Tip For best results, initiate chat on the files or code changes. @leynos I've already provided a comprehensive fix for this code duplication issue earlier in this thread. You can see my previous response where I suggested:
The solution eliminates approximately 60% of the duplicated code while maintaining test clarity and identical functionality. Would you like me to:
Please let me know if you'd like me to elaborate on the earlier solution or if you're looking for something different! |
Add a publish.strip_patches setting to control removal of [patch.crates-io] entries in the staged workspace Cargo.toml during the publish command. - "all" removes the entire [patch.crates-io] section. - "per-crate" removes patch entries for publishable crates only. - false leaves patch entries untouched. This preserves or removes patch overrides based on configuration to ensure correct dependency resolution during package publication. Includes comprehensive BDD and unit tests verifying behavior and updates documentation to explain patch stripping configuration and staging manifest normalization. Closes: #<issue_if_any> Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
…andling - Consolidate patch entry removals with clearer helper functions - Add error handling for manifest file read/write operations - Clean up unused and duplicated code in patch section management - Adjust logger usage in publish_execution module - Enhance tests for patch stripping strategies and patch table cleanup - Minor improvements for code clarity and maintainability in publish commands Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
…eanup Refactor `_apply_strip_patch_strategy` in `publish.py` to separate manifest validation, patch application, and cleanup logic. Introduce `_validate_and_load_manifest`, `_apply_strategy_to_patches`, and `_cleanup_empty_patch_tables` helper functions for clearer responsibilities. Enhance BDD and unit tests around patch stripping strategies for better coverage and reuse with the `_apply_strategy_and_parse` helper. Adjust logic handling to support boolean and string strategies with improved error handling. This restructuring improves maintainability and clarity of the patch stripping feature for publish manifests. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
Replaced the inline condition checking for cargo 'check' and 'test' commands with a call to the helper function '_is_cargo_action_command'. This improves code readability and maintainability in test_publish_steps.py. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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: 2
📜 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 (2)
lading/commands/publish.py(3 hunks)tests/bdd/steps/test_publish_steps.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
lading/commands/publish.pytests/bdd/steps/test_publish_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/bdd/steps/test_publish_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/bdd/steps/test_publish_steps.py
🧬 Code graph analysis (2)
lading/commands/publish.py (2)
lading/commands/publish_execution.py (5)
_CommandRunner(52-62)_invoke(81-94)_normalise_cmd_mox_command(185-195)_should_use_cmd_mox_stub(107-110)_split_command(97-104)lading/commands/publish_plan.py (5)
PublishPlan(22-34)PublishPlanError(17-18)_append_section(213-223)_format_plan(226-257)publishable_names(32-34)
tests/bdd/steps/test_publish_steps.py (2)
tests/bdd/steps/test_common_steps.py (1)
_run_cli(24-49)tests/unit/conftest.py (1)
staging_root(184-186)
🪛 GitHub Actions: CI
lading/commands/publish.py
[warning] 22-39: I001 Import block is un-sorted or un-formatted. Organize imports.
[error] 376-376: E501 Line too long (156 > 88).
⏰ 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 (9)
tests/bdd/steps/test_publish_steps.py (5)
12-33: TOML typing and runtime fallback look correctUsing
tomlkit.parseonly in tests and gatingTOMLDocumentbehindTYPE_CHECKINGwith anAnyfallback keeps type-checkers happy without adding runtime dependencies to the test harness. The pattern is consistent and clear.
314-333: Preflight override regex + parsing are robust and readableThe regex-based
@givenstep with named groups and the updated implementation that:
- treats
exit_codeasstrat the boundary and converts viaint(exit_code), and- enforces non-empty
tokenswith a clearAssertionErrormessage,makes this step more explicit and user-friendly while matching prior behaviour.
336-345: Plan header assertion now resilient to configuration changesSwitching to
lines[1].startswith("Strip patch strategy:")keeps the test stable if the strategy value formatting changes, while still guaranteeing the expected header is present.
347-373: Staged manifest loading and patch-table extraction align with publish flow
_load_staged_manifestreuses_publish_plan_linesand_extract_staging_root_from_plan, asserts the stagedCargo.tomlexists, and parses viaparse_toml, giving good failure signals when the manifest is missing._get_patch_entriescarefully validates both the"patch"and"crates-io"tables are mappings and returns a shallowdictcopy, avoiding accidental mutation of the TOML document.then_publish_manifest_has_no_patch_sectioncorrectly asserts that there are no patch entries via_get_patch_entries(document) == {}.All three helpers are cohesive and mirror the production patch-stripping behaviour from
lading/commands/publish.py.
375-400: Patch-entry assertion helpers are simple and composable
_split_nameshandles comma-separated lists with trimming and empty-string filtering.then_publish_manifest_omits_entriesandthen_publish_manifest_retains_entriesare thin, readable wrappers that assert absence/presence of each named crate in the staged[patch.crates-io]table.The helpers keep BDD steps declarative and avoid duplicated TOML plumbing; no further refactor seems necessary here.
lading/commands/publish.py (4)
313-340: Manifest load/write helpers are robust and aligned with error-handling guidelines
_load_manifest_documentwraps file I/O in targetedFileNotFoundError/PermissionError/OSErrorhandlers and surfacesTOMLKitErrorasPublishPreparationError, preserving causal chains usingraise … from …._write_manifest_documentensures a trailing newline and wrapswrite_textin anOSErrorguard, again raisingPublishPreparationErrorwith useful context.This matches the project’s exception-handling guidelines and should give clear, actionable messages for manifest issues.
342-365: Patch-table resolution and per-crate removal are correct and defensive
_remove_per_crate_entriesdeduplicatescrate_namesviadict.fromkeysand reports whether any entries were removed, which avoids unnecessary writes and keeps behaviour deterministic._resolve_patch_tablescleanly validates both"patch"and"crates-io"tables as mutable mappings before proceeding, returningNonewhen patch stripping should be a no-op (e.g., no patch section configured).These helpers make the subsequent strategy application code much simpler while guarding against unexpected manifest structures.
401-440: Strip-patch strategy application is clear and respects configuration semantics
_apply_strategy_to_patchescleanly dispatches between"all"(remove the entirecrates-iotable) and"per-crate"(remove only entries forplan.publishable_names), returning a boolean to indicate whether any changes occurred and raisingPublishPreparationErrorfor unsupported strategies rather than silently ignoring them._apply_strip_patch_strategycomposes_validate_and_load_manifest,_apply_strategy_to_patches,_cleanup_empty_patch_tables, and_write_manifest_documentwith early returns when:
- strip-patches is disabled,
- no staged manifest exists,
- no
[patch.crates-io]table is present, or- the strategy makes no effective changes.
This keeps cyclomatic complexity under control while making the control flow easy to follow. No further structural changes seem necessary here.
497-504: run() wiring for patch-stripping is in the right placeInvoking
_apply_strip_patch_strategyimmediately afterprepare_workspaceensures that:
- patch stripping is applied only to the staged manifest (never the source workspace), and
- the same
strip_patchessetting is used both for mutation and for rendering via_format_plan.This preserves existing behaviour while adding the new configuration-dependent manifest rewrite step.
- Introduce _PatchStrategyTestSetup dataclass to encapsulate test parameters - Simplify and unify test helper function _apply_strategy_and_parse to accept the dataclass instance - Maintain existing test coverage and behavior while improving code clarity and maintainability Also, improve docstring formatting in publish.py _validate_and_load_manifest function. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
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: 2
♻️ Duplicate comments (1)
lading/commands/publish.py (1)
22-39: Organize imports to resolve pipeline failure.The import block at lines 22-39 is flagged as un-sorted by the linter. The
from lading.*imports should be alphabetically ordered within their group.Apply this diff to sort the imports:
from lading import config as config_module from lading.commands.publish_diagnostics import _append_compiletest_diagnostics from lading.commands.publish_execution import ( _CommandRunner, _invoke, _normalise_cmd_mox_command as _execution_normalise_cmd_mox_command, _should_use_cmd_mox_stub as _execution_should_use_cmd_mox_stub, _split_command as _execution_split_command, ) from lading.commands.publish_plan import ( PublishPlan, PublishPlanError as _PlanPublishPlanError, _append_section as _plan_append_section, _format_plan, plan_publication, ) from lading.utils.path import normalise_workspace_root from lading.workspace import metadata as _metadata_moduleRun
make lintor your project's import sorter to ensure the exact ordering matches your configuration.
🧹 Nitpick comments (1)
tests/unit/test_publish_patch_strategy.py (1)
18-26: Addslots=Trueto internal dataclass.The
_PatchStrategyTestSetupdataclass is internal-only and should useslots=Truefor better memory efficiency and performance, per coding guidelines.Apply this diff:
-@dataclass(frozen=True) +@dataclasses.dataclass(frozen=True, slots=True) class _PatchStrategyTestSetup: """Parameters for patch strategy test setup."""
📜 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 (2)
lading/commands/publish.py(3 hunks)tests/unit/test_publish_patch_strategy.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
lading/commands/publish.pytests/unit/test_publish_patch_strategy.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_publish_patch_strategy.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_publish_patch_strategy.py
🧬 Code graph analysis (2)
lading/commands/publish.py (3)
lading/commands/publish_execution.py (5)
_CommandRunner(52-62)_invoke(81-94)_normalise_cmd_mox_command(185-195)_should_use_cmd_mox_stub(107-110)_split_command(97-104)lading/commands/publish_plan.py (5)
PublishPlan(22-34)PublishPlanError(17-18)_append_section(213-223)_format_plan(226-257)publishable_names(32-34)lading/utils/path.py (1)
normalise_workspace_root(10-16)
tests/unit/test_publish_patch_strategy.py (3)
lading/workspace/models.py (1)
WorkspaceCrate(59-69)lading/commands/publish_plan.py (2)
PublishPlan(22-34)publishable_names(32-34)lading/commands/publish.py (1)
_apply_strip_patch_strategy(421-444)
🪛 GitHub Actions: CI
lading/commands/publish.py
[error] 22-39: Ruff: Import block is un-sorted or un-formatted. Organize imports.
tests/unit/test_publish_patch_strategy.py
[error] 3-12: Ruff: Import block is un-sorted or un-formatted. Organize imports.
[error] 5-5: Ruff: ICN003 Members of dataclasses should not be imported explicitly.
[error] 6-6: Ruff: TC003 Move standard library import pathlib.Path into a type-checking block.
[error] 70-70: Ruff: ANN202 Missing return type annotation for private function _apply_strategy_and_parse.
⏰ 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
… test setup class - Changed imports from 'from dataclasses import dataclass' to 'import dataclasses' - Updated @DataClass decorator to use 'dataclasses.dataclass' - Adjusted type annotations for 'tmp_path' and 'make_plan_factory' to use forward references - Organized TYPE_CHECKING import of Path under conditional block These changes improve consistency and clarity in the test code's type annotations. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…rse function Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationtests/unit/test_publish_patch_strategy.py: What lead to degradation?The module contains 2 functions with similar structure: test_strip_patches_all_removes_patch_section,test_strip_patches_disabled_keeps_section Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
Tip For best results, initiate chat on the files or code changes. @leynos I've already provided a comprehensive solution for this code duplication issue earlier in this thread. To recap, I provided a revised fix that addresses both the code duplication and the parameter count concern (where the initial helper had 5 arguments, exceeding the threshold of 4). The Solution I Already ProvidedThe approach uses a dataclass to encapsulate test setup parameters, reducing the helper function to just 1 argument:
This solution:
The complete code examples and AI coding agent prompt are in my earlier response in this thread (search for "_PatchStrategyTestSetup" in the conversation above). Would you like me to:
The dataclass-based solution I provided earlier is ready to use and addresses all the code health issues flagged by CodeScene! 🐰 |
… publish commands and tests - Consolidated multiple import statements in lading/commands/publish.py for clarity - Changed dataclasses import alias to dc in test_publish_patch_strategy.py - Added explicit type imports and annotations for better type clarity in tests - Minor formatting adjustments in test code to enhance readability Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
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: 0
♻️ Duplicate comments (1)
lading/commands/publish.py (1)
24-47: Consolidate imports from the same modules to fix I001 violation.Multiple separate import statements from
lading.commands.publish_executionandlading.commands.publish_planshould be consolidated into single import blocks per module. This fragmentation is likely causing the I001 lint error flagged in previous reviews.Apply this diff to consolidate the imports:
-from lading.commands.publish_execution import ( - _CommandRunner, - _invoke, -) -from lading.commands.publish_execution import ( - _normalise_cmd_mox_command as _execution_normalise_cmd_mox_command, -) -from lading.commands.publish_execution import ( - _should_use_cmd_mox_stub as _execution_should_use_cmd_mox_stub, -) -from lading.commands.publish_execution import ( - _split_command as _execution_split_command, -) -from lading.commands.publish_plan import ( - PublishPlan, - _format_plan, - plan_publication, -) -from lading.commands.publish_plan import ( - PublishPlanError as _PlanPublishPlanError, -) -from lading.commands.publish_plan import ( - _append_section as _plan_append_section, -) +from lading.commands.publish_execution import ( + _CommandRunner, + _invoke, + _normalise_cmd_mox_command as _execution_normalise_cmd_mox_command, + _should_use_cmd_mox_stub as _execution_should_use_cmd_mox_stub, + _split_command as _execution_split_command, +) +from lading.commands.publish_plan import ( + PublishPlan, + PublishPlanError as _PlanPublishPlanError, + _append_section as _plan_append_section, + _format_plan, + plan_publication, +)
🧹 Nitpick comments (2)
lading/commands/publish.py (2)
377-401: Consider a type alias for the complex return type.The return type is quite verbose and used only once. While correct, a module-level type alias would improve readability and align with the guideline to "define type aliases with the type statement" for complex signatures.
Add a type alias near the top of the file (after line 57):
type _ManifestValidation = tuple[ TOMLDocument, tuple[cabc.MutableMapping[str, typ.Any], cabc.MutableMapping[str, typ.Any]], ] | NoneThen simplify the function signature:
def _validate_and_load_manifest( staging_root: Path, strategy: StripPatchesSetting -) -> ( - tuple[ - TOMLDocument, - tuple[cabc.MutableMapping[str, typ.Any], cabc.MutableMapping[str, typ.Any]], - ] - | None -): +) -> _ManifestValidation:
352-361: Optional: clarify the deduplication intent.
dict.fromkeys(crate_names)is used to deduplicate crate names while preserving insertion order. While this is a valid Python idiom, a brief inline comment would make the intent immediately clear.def _remove_per_crate_entries( crates_io: cabc.MutableMapping[str, typ.Any], crate_names: cabc.Iterable[str], ) -> bool: """Remove entries for ``crate_names`` and return ``True`` when modified.""" removed = False + # Deduplicate while preserving order for crate in dict.fromkeys(crate_names): if crates_io.pop(crate, None) is not None: removed = True return removed
📜 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 (2)
lading/commands/publish.py(3 hunks)tests/unit/test_publish_patch_strategy.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/test_publish_patch_strategy.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
lading/commands/publish.py
🧬 Code graph analysis (1)
lading/commands/publish.py (3)
lading/commands/publish_execution.py (5)
_CommandRunner(52-62)_invoke(81-94)_normalise_cmd_mox_command(185-195)_should_use_cmd_mox_stub(107-110)_split_command(97-104)lading/commands/publish_plan.py (5)
PublishPlan(22-34)_format_plan(226-257)PublishPlanError(17-18)_append_section(213-223)publishable_names(32-34)lading/utils/path.py (1)
normalise_workspace_root(10-16)
⏰ 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 (3)
lading/commands/publish.py (3)
323-349: LGTM: Robust manifest I/O with appropriate error handling.The manifest loading and writing functions correctly handle I/O failures (FileNotFoundError, PermissionError, OSError) and parsing errors (TOMLKitError), wrapping them in PublishPreparationError with clear messages while preserving causal chains using
raise...from.
431-454: LGTM: Clean orchestration of patch-stripping logic.
_apply_strip_patch_strategydelegates to well-factored helpers and uses early returns to avoid unnecessary work. The flow (validate → apply strategy → cleanup → persist) is clear and easy to follow. This addresses the complexity concerns raised in earlier reviews.
512-516: LGTM: Patch-stripping correctly integrated into publish flow.The patch-stripping is invoked after workspace preparation but before plan formatting, ensuring the staged manifest is modified in-place as intended. The placement preserves workspace integrity while allowing the plan to reflect the actual staged state.
…licate crate names - Added _ManifestValidation type alias to clarify return type in _validate_and_load_manifest - Deduplicated crate names in _remove_per_crate_entries using dict.fromkeys for deterministic processing These changes improve code readability and maintainability without altering behaviour. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
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 (1)
lading/commands/publish.py (1)
1-1: Runruff formatto fix code style issues.The CI pipeline reports that
ruff format --checkwould reformat this file. You must runruff format lading/commands/publish.pylocally and commit the reformatted version before this PR can merge.
📜 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 (1)
lading/commands/publish.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
lading/commands/publish.py
🧬 Code graph analysis (1)
lading/commands/publish.py (3)
lading/commands/publish_execution.py (5)
_CommandRunner(52-62)_invoke(81-94)_normalise_cmd_mox_command(185-195)_should_use_cmd_mox_stub(107-110)_split_command(97-104)lading/commands/publish_plan.py (5)
PublishPlan(22-34)_format_plan(226-257)PublishPlanError(17-18)_append_section(213-223)publishable_names(32-34)lading/utils/path.py (1)
normalise_workspace_root(10-16)
🪛 GitHub Actions: CI
lading/commands/publish.py
[error] 1-1: ruff format --check would reform 1 file. Run 'ruff format' to fix code style issues in this file.
⏰ 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 (2)
lading/commands/publish.py (2)
328-454: LGTM: TOML manifest utilities are well-implemented.The patch-stripping utilities are well-structured with comprehensive error handling, clear type annotations, and appropriate defensive guards. The logic correctly:
- Loads and parses manifests with tomlkit to preserve formatting
- Validates patch tables before manipulation
- Applies "all" and "per-crate" strategies
- Cleans up empty tables after removals
- Writes back preserving TOML trivia
Past review concerns about I/O error handling and docstring length were properly addressed in earlier commits.
512-516: LGTM: Integration of patch-stripping is correct.The patch-stripping flow is correctly integrated into the publish run: it executes after workspace staging (line 511) and before plan formatting (lines 517-519), ensuring the staged manifest is modified in place before final output. The configuration value
active_configuration.publish.strip_patchesis properly threaded through.
Correct parentheses placement and indentation for the _ManifestValidation type alias to improve code readability and maintain style consistency in lading/commands/publish.py. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Gates Failed
(1 new file with code health below 10.00)
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| New code is healthy | Violations | Code Health Impact | |
|---|---|---|---|
| test_publish_patch_strategy.py | 1 rule | 9.39 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| test_publish_patch_strategy.py | 1 advisory rule | 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationtests/unit/test_publish_patch_strategy.py: What lead to degradation?The module contains 2 functions with similar structure: test_strip_patches_all_removes_patch_section,test_strip_patches_disabled_keeps_section Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5357 @leynos Great news! 🎉 The code duplication issue in Looking at the current state of the file, I can see that the refactoring has been successfully applied: Current State (Already Fixed)The file now contains:
@dc.dataclass(frozen=True)
class _PatchStrategyTestSetup:
"""Parameters for patch strategy test setup."""
tmp_path: Path
make_plan_factory: typ.Callable[[Path, tuple[str, ...]], publish.PublishPlan]
patch_entries: str
publishable_names: tuple[str, ...]
strategy: str | bool
def _apply_strategy_and_parse(setup: _PatchStrategyTestSetup) -> TOMLDocument:
"""Set up workspace, apply patch strategy, and return parsed document."""
# ... common setup logic ...
Result✅ Code duplication eliminated The static analysis tool may be reporting stale information or may need to re-scan the updated code. The duplication issue has been successfully addressed! 🐰 |
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1361 🏁 Script executed: Length of output: 7744 🏁 Script executed: Length of output: 2047 🏁 Script executed: Length of output: 1990 @leynos No, none of these three suggestions have been resolved yet: 1. ❌ TOML manifest helpers still in publish.pyThe TOML manipulation and patch-stripping helpers remain in
The file is currently 695 lines with 29 functions, so extracting these helpers to a dedicated module (e.g., 2. ❌ Duplicate BDD test helpers not extractedMultiple BDD step files independently import and use
While there is a 3. ❌ Import aliasing still verboseThe import section of
This could be simplified by re-exporting these functions directly in the source modules or consolidating the imports. Would you like me to create issues for these refactoring tasks or provide implementation guidance for any of them? |
1 participant
Core Functionality
Configuration
Testing
Documentation
Miscellaneous
Why
This change enables precise control over how local patch entries are treated during crate packaging and publishing, avoiding unintended dependencies on local overrides and improving reproducibility across environments.
How to test
Breaking changes
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/20af1433-5a94-481c-896e-b29acc23ccb1
Summary by Sourcery
Implement configurable patch-stripping in the publish workflow by loading the staged manifest with tomlkit, removing patch.crates-io entries according to the selected strategy, and updating logging, documentation, and tests accordingly
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.