Refactor manifest helpers; centralize publish_manifest and exports - #42
Conversation
Reviewer's GuideRefactors publish manifest handling into a dedicated helper module, switches publish command and tests to consume those helpers and new public utilities, and exposes selected execution/plan helpers as part of the public API for reuse. Sequence diagram for centralized manifest patch-stripping workflowsequenceDiagram
actor User as "CLI user"
participant Publish as "publish.py (publish command)"
participant Manifest as "publish_manifest module"
participant FS as "Filesystem"
User->>Publish: "Invoke publish command"
Publish->>Manifest: "_apply_strip_patch_strategy(staging_root, plan, strategy)"
alt "Strategy is False"
Manifest-->>Publish: "Return (no-op)"
else "Strategy enabled"
Manifest->>Manifest: "_validate_and_load_manifest(staging_root, strategy)"
alt "Manifest missing or no patch tables"
Manifest-->>Publish: "Return (no-op)"
else "Manifest and patch tables available"
Manifest->>FS: "Read Cargo.toml via _load_manifest_document(manifest_path)"
FS-->>Manifest: "Manifest text or error"
alt "Read or parse error"
Manifest->>Publish: "Raise PublishPreparationError"
else "Manifest loaded successfully"
Manifest->>Manifest: "_resolve_patch_tables(document)"
Manifest->>Manifest: "_apply_strategy_to_patches(strategy, patch_table, crates_io, plan.publishable_names)"
alt "No patches removed (modified == False)"
Manifest-->>Publish: "Return (no changes)"
else "Patches removed (modified == True)"
Manifest->>Manifest: "_cleanup_empty_patch_tables(document, patch_table, crates_io)"
Manifest->>FS: "_write_manifest_document(manifest_path, document)"
FS-->>Manifest: "Manifest written"
Manifest-->>Publish: "Return (patches stripped)"
end
end
end
end
Publish-->>User: "Publish completed with updated manifest"
Class diagram for publish_manifest and newly exported helper utilitiesclassDiagram
class PublishPreparationError {
<<exception>>
"Inherits from RuntimeError"
}
class publish_manifest {
<<module>>
"+StripPatchesSetting : type alias"
"+_load_manifest_document(manifest_path: Path) TOMLDocument"
"+_write_manifest_document(manifest_path: Path, document: TOMLDocument) None"
"+_remove_per_crate_entries(crates_io: MutableMapping[str, Any], crate_names: Iterable[str]) bool"
"+_resolve_patch_tables(document: TOMLDocument) tuple[MutableMapping[str, Any], MutableMapping[str, Any]] | None"
"+_validate_and_load_manifest(staging_root: Path, strategy: StripPatchesSetting) _ManifestValidation"
"+_cleanup_empty_patch_tables(document: TOMLDocument, patch_table: MutableMapping[str, Any], crates_io: MutableMapping[str, Any]) None"
"+_apply_strategy_to_patches(strategy: StripPatchesSetting, patch_table: MutableMapping[str, Any], crates_io: MutableMapping[str, Any], publishable_names: tuple[str, ...]) bool"
"+_apply_strip_patch_strategy(staging_root: Path, plan: PublishPlan, strategy: StripPatchesSetting) None"
}
class publish_execution {
<<module>>
"+_CommandRunner"
"+_invoke(...)"
"-_split_command(command: str) list[str]"
"-_should_use_cmd_mox_stub(env: dict[str, str]) bool"
"-_normalise_cmd_mox_command(args: list[str]) list[str]"
"+split_command(command: str) list[str]"
"+should_use_cmd_mox_stub(env: dict[str, str]) bool"
"+normalise_cmd_mox_command(args: list[str]) list[str]"
}
class publish_plan {
<<module>>
"+PublishPlan"
"+PublishPlanError"
"-_append_section(plan: PublishPlan, title: str, lines: list[str]) None"
"-_format_plan(plan: PublishPlan, strip_patches: StripPatchesSetting) str"
"+append_section(plan: PublishPlan, title: str, lines: list[str]) None"
"+format_plan(plan: PublishPlan, strip_patches: StripPatchesSetting) str"
"+plan_publication(...) PublishPlan"
}
class publish {
<<module>>
"+StripPatchesSetting : type alias"
"+metadata_module : module alias"
"+PublishPlanError : type alias"
"+_normalise_cmd_mox_command(args: list[str]) list[str]"
"+_should_use_cmd_mox_stub(env: dict[str, str]) bool"
"+_split_command(command: str) list[str]"
"+_append_section(plan: PublishPlan, title: str, lines: list[str]) None"
"+_format_plan(plan: PublishPlan, strip_patches: StripPatchesSetting) str"
}
publish_manifest --> PublishPreparationError : "raises"
publish --> publish_manifest : "imports _apply_strip_patch_strategy and PublishPreparationError"
publish --> publish_execution : "imports public cmd-mox helpers"
publish --> publish_plan : "imports PublishPlan, PublishPlanError, append_section, format_plan"
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. Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRefactor publish workflow by extracting manifest/patch-stripping and execution helpers into dedicated modules, expose utility aliases across publish modules, centralise TOML test utilities, add BDD test registration, and extend documentation with a "Publish data flow" diagram. Also remove Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(245,250,255)
participant CLI as User CLI
participant Pub as publish
participant Plan as publish_plan
participant Mani as publish_manifest
participant Exec as publish_execution
end
CLI->>Pub: invoke publish command
Pub->>Plan: build PublishPlan
Plan-->>Pub: PublishPlan
Pub->>Mani: apply_strip_patch_strategy
Mani->>Mani: load_manifest_document
Mani->>Mani: resolve_patch_tables
Mani->>Mani: apply_strategy_to_patches
Mani->>Mani: cleanup_empty_patch_tables
Mani->>Mani: write_manifest_document
Mani-->>Pub: patches applied
Pub->>Plan: append_section / format_plan
Plan-->>Pub: formatted plan
Pub->>Exec: split_command (normalise)
Exec-->>Pub: normalised args
Pub->>Exec: invoke (execute plan)
Exec-->>Pub: success / error
Pub-->>CLI: publish result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.py⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (2)lading/testing/toml_utils.py (1)
lading/commands/publish_manifest.py (2)
🔍 Remote MCPExcellent! I now have comprehensive context. Let me compile this into a clear summary for the PR review. Summary of Additional Context for PR ReviewBased on the repository documentation and implementation details, here is the relevant context for reviewing this refactoring PR: Publish Command Workflow ContextThe StripPatchesSetting and Patch StrippingThe Error Handling StrategyThe publish workflow uses three main error types: Testing InfrastructureThe testing infrastructure for the publish command uses BDD with cmd-mox Helpers Being Re-exportedThe three cmd-mox helpers ( Key Review Focus Areas
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (20)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Extract patch stripping and manifest manipulation logic into a new module lading.commands.publish_manifest to improve separation of concerns and maintainability. Removed duplicated code and adjusted imports accordingly. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
82b4b74 to
3e40cf6
Compare
![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 they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/bdd/steps/test_publish_steps.py:151-157` </location>
<code_context>
argument_tuple = tuple(args)
- if _is_cargo_action_command(program, argument_tuple):
- return f"cargo::{argument_tuple[0]}", argument_tuple[1:]
+ if program == "cargo":
+ normalised_program, invocation_args = publish._normalise_cmd_mox_command(
+ program,
+ argument_tuple,
+ )
+ return normalised_program, tuple(invocation_args)
return program, argument_tuple
</code_context>
<issue_to_address>
**suggestion (testing):** Extend preflight expectation tests to cover cmd-mox normalization edge cases via normalise_cmd_mox_command
Since _resolve_preflight_expectation now relies on publish._normalise_cmd_mox_command instead of _is_cargo_action_command, the mapping from raw cargo invocations to cmd-mox program/args is more nuanced. To prevent regressions, please add or extend tests around _resolve_preflight_expectation to cover:
- existing cases (cargo check, cargo test)
- other common subcommands (e.g., clippy, fmt, build, doc)
- commands with extra flags/args (e.g., cargo test --package foo -- --ignored)
A parametrized test here would make the behavior explicit and help ensure future changes to normalise_cmd_mox_command don’t silently break the preflight stubbing in these BDD tests.
Suggested implementation:
```python
from tests.bdd import toml_utils
from . import config_fixtures as _config_fixtures # noqa: F401
from . import manifest_fixtures as _manifest_fixtures # noqa: F401
@pytest.mark.parametrize(
"command, expected_program, expected_args_prefix",
[
# Existing/common cases
(("cargo", "check"), "cargo::check", ()),
(("cargo", "test"), "cargo::test", ()),
# Other common subcommands
(("cargo", "clippy"), "cargo::clippy", ()),
(("cargo", "fmt"), "cargo::fmt", ()),
(("cargo", "build"), "cargo::build", ()),
(("cargo", "doc"), "cargo::doc", ()),
# Commands with extra flags/args (including `--` separator)
(
("cargo", "test", "--package", "foo", "--", "--ignored"),
"cargo::test",
("--package", "foo", "--", "--ignored"),
),
],
)
def test_resolve_preflight_expectation_normalises_cargo_commands(
command: tuple[str, ...],
expected_program: str,
expected_args_prefix: tuple[str, ...],
) -> None:
"""Ensure _resolve_preflight_expectation stays in sync with normalise_cmd_mox_command.
These cases cover common cargo subcommands as well as invocations that
include additional flags and a double-dash argument separator.
"""
program, args_prefix = _resolve_preflight_expectation(command)
assert program == expected_program
assert args_prefix == expected_args_prefix
```
1. Ensure that `_resolve_preflight_expectation` is in scope in this file:
- If it is defined in this same module (as a helper above the step definitions), the test can call it directly as shown.
- If it lives in another module (e.g., `lading.commands.publish` or a helper module), add an explicit import near the other imports, for example:
`from lading.commands.publish import _resolve_preflight_expectation` (or the correct path), and keep the test body unchanged.
2. The expected values (`"cargo::check"`, etc.) are based on the previous `_is_cargo_action_command` behavior. If `publish._normalise_cmd_mox_command` intentionally changes the mapping (e.g., different program naming or argument handling), please adjust `expected_program` and `expected_args_prefix` in the parametrization to match the actual, desired normalization.
3. If there are existing tests that already cover `cargo check` and `cargo test` in this file, you may want to:
- Either remove those older, more specific tests to avoid duplication, or
- Fold their expectations into this parametrized test (or vice versa) to keep the test suite DRY and consistent.
</issue_to_address>
### Comment 2
<location> `tests/bdd/steps/manifest_fixtures.py:10` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 3
<location> `tests/bdd/steps/test_common_steps.py:13` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 4
<location> `tests/bdd/steps/test_publish_steps.py:15` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 5
<location> `lading/commands/publish_manifest.py:109-111` </location>
<code_context>
def _validate_and_load_manifest(
staging_root: Path, strategy: StripPatchesSetting
) -> _ManifestValidation:
"""Load and validate the manifest for patch stripping.
Returns the document and patch tables when applicable, or None if
stripping should be skipped.
"""
if strategy is False:
return None
manifest_path = staging_root / "Cargo.toml"
if not manifest_path.exists():
return None
document = _load_manifest_document(manifest_path)
patch_tables = _resolve_patch_tables(document)
if patch_tables is None:
return None
return document, patch_tables
</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 None if patch_tables is None else (document, patch_tables)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Added a detailed Mermaid graph diagram illustrating the data flow of the `lading publish` command and its related modules. This visual aid helps in understanding the publish command's internal workflow and components, improving the documentation for developers and users interacting with the publishing process. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/manifest_fixtures.py (1)
87-91: Remove redundant existence check.The existence check at lines 87-89 duplicates the validation already performed by
toml_utils.load_manifest(line 91). Remove lines 87-89.Apply this diff:
- if not manifest_path.exists(): - message = f"Workspace manifest not found: {manifest_path}" - raise AssertionError(message) names = [name.strip() for name in crate_names.split(",") if name.strip()] document = toml_utils.load_manifest(manifest_path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
.coderabbit.yaml(0 hunks)docs/lading-design.md(1 hunks)lading/commands/publish.py(1 hunks)lading/commands/publish_execution.py(1 hunks)lading/commands/publish_manifest.py(1 hunks)lading/commands/publish_plan.py(1 hunks)tests/bdd/steps/config_fixtures.py(3 hunks)tests/bdd/steps/manifest_fixtures.py(3 hunks)tests/bdd/steps/test_common_steps.py(4 hunks)tests/bdd/steps/test_publish_steps.py(3 hunks)tests/bdd/toml_utils.py(2 hunks)
💤 Files with no reviewable changes (1)
- .coderabbit.yaml
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*.py: Python files must use snake_case filenames (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants must use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single leading underscore
Use typing everywhere and maintain full static type coverage
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only usage
Avoid Any; prefer precise types (TypeVar, Protocol, Literal, Union); use typing.cast only when necessary with justification; use object for unknown opaque values
Be explicit with return types (e.g., -> None, -> str) for all public functions and methods
Favor immutability: prefer tuples to lists and MappingProxyType for read-only mappings; document third-party frozendict if used
Use# pyright: ignoresparingly and include an explanation comment when used
Avoid side effects at import time (no global state mutation or actions on import)
Use docstrings (NumPy format) for public functions, classes, and modules
Explain tricky code with inline comments
**/*.py: Prefer context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual try/finally blocks
Use contextlib.contextmanager (@contextmanager) to implement simple, linear setup/teardown context managers
Implement a class with enter and exit for context managers that require internal state or more complex lifecycle handling
For file I/O, prefer with open(...) as ... over open()/try/finally/close patterns
Choose @contextmanager when control flow is linear and no persistent state is needed
Choose a class-based context manager when there is internal state, lifecycle methods, re-entry, or advanced context needs
**/*.py: Exception classes must end with the suffix 'Error' (N818)
Prefer specific built-ins (e.g., TypeError, ValueError) or domain exceptions over raising Exception directly (TRY003/TRY004)
Preserve causal chains...
Files:
tests/bdd/steps/test_common_steps.pytests/bdd/toml_utils.pytests/bdd/steps/test_publish_steps.pylading/commands/publish_execution.pytests/bdd/steps/manifest_fixtures.pylading/commands/publish_manifest.pylading/commands/publish.pylading/commands/publish_plan.pytests/bdd/steps/config_fixtures.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/bdd/steps/test_common_steps.pytests/bdd/toml_utils.pytests/bdd/steps/test_publish_steps.pylading/commands/publish_execution.pytests/bdd/steps/manifest_fixtures.pylading/commands/publish_manifest.pylading/commands/publish.pylading/commands/publish_plan.pytests/bdd/steps/config_fixtures.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_common_steps.pytests/bdd/toml_utils.pytests/bdd/steps/test_publish_steps.pytests/bdd/steps/manifest_fixtures.pytests/bdd/steps/config_fixtures.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_common_steps.pytests/bdd/toml_utils.pytests/bdd/steps/test_publish_steps.pytests/bdd/steps/manifest_fixtures.pytests/bdd/steps/config_fixtures.py
{README.md,docs/**}
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate documentation: keep README.md or a docs/ directory near reusable packages and include usage examples
Files:
docs/lading-design.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/lading-design.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/lading-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/lading-design.md
🧬 Code graph analysis (5)
tests/bdd/steps/test_common_steps.py (1)
tests/bdd/toml_utils.py (1)
load_manifest(65-70)
tests/bdd/steps/test_publish_steps.py (2)
lading/commands/publish_execution.py (1)
_normalise_cmd_mox_command(185-195)tests/bdd/toml_utils.py (1)
load_manifest(65-70)
tests/bdd/steps/manifest_fixtures.py (1)
tests/bdd/toml_utils.py (1)
load_manifest(65-70)
lading/commands/publish_manifest.py (2)
lading/commands/publish_plan.py (1)
publishable_names(32-34)tests/unit/conftest.py (1)
staging_root(184-186)
tests/bdd/steps/config_fixtures.py (1)
tests/bdd/toml_utils.py (1)
load_or_create_document(28-32)
🔍 Remote MCP Deepwiki
Summary — additional context relevant to reviewing this PR
-
The new module lading/commands/publish_manifest.py centralizes Cargo.toml staging: it implements robust manifest load/write helpers, patch-crates-io resolution/cleanup, a PublishPreparationError, and an orchestration function _apply_strip_patch_strategy that applies the configured strip-patches strategy.
-
strip_patches semantics (must be preserved by the refactor): configuration.publish.strip_patches accepts "all" | "per-crate" | false. "all" removes the entire [patch.crates-io] before preflight/validation, "per-crate" removes per-crate entries during staging/publish, false leaves patches unchanged — verify the new module is invoked at the same point in publish.run as before.
-
publish.py now delegates previously in-file manifest helpers to publish_manifest; review must confirm exported names, error types, and public aliases still match call sites (notably PublishPreparationError has moved/new aliasing). Also confirm call sites use the new _apply_strip_patch_strategy import.
-
publish_execution.py and publish_plan.py intentionally expose new public aliases used across the codebase/tests: normalise_cmd_mox_command / should_use_cmd_mox_stub / split_command (publish_execution) and append_section / format_plan (publish_plan). Verify all and symbol names match what other modules/tests import.
-
Tests: BDD/unit tests were changed to use centralized toml utilities (tests/bdd/toml_utils.py: load_manifest, load_workspace_manifest, load_crate_manifest). Those helpers assert manifest existence and centralize parsing — check compatibility between test helpers’ semantics and publish_manifest’s error messages/exceptions (missing/parse/write errors should map cleanly to PublishPreparationError where expected).
-
Cmd-mox / preflight interaction: publish uses a _CommandRunner protocol and routes through _invoke/_invoke_via_cmd_mox when LADING_USE_CMD_MOX_STUB is enabled; publish_execution exports the cmd-mox normalization helpers — confirm tests and BDD stubs still normalize/record cargo/git invocations as before.
Files / symbols to spot‑check in the review
- lading/commands/publish_manifest.py: _load_manifest_document, _write_manifest_document, _resolve_patch_tables, _apply_strip_patch_strategy, PublishPreparationError, StripPatchesSetting.
- lading/commands/publish.py: call sites where strip-patch logic is invoked and where PublishPreparationError (formerly in-file) may be referenced.
- lading/commands/publish_execution.py and lading/commands/publish_plan.py: all and exported alias names.
- tests/bdd/toml_utils.py and updated test fixtures/steps that now call toml_utils.load_manifest.
(References above drawn from the project wiki/docs introspection.)
🔇 Additional comments (21)
lading/commands/publish_plan.py (1)
260-272: LGTM: Public aliases correctly expose internal helpers.The aliasing pattern cleanly exposes
_append_sectionand_format_planas public interfaces without modifying their implementations. The__all__export list is correctly updated to include both the private and public names, maintaining backward compatibility whilst allowing external modules to import the conventional (non-underscore) names.lading/commands/publish_execution.py (1)
470-480: LGTM: Execution helpers properly exposed for external use.The public aliases follow the established pattern and correctly expose cmd-mox integration helpers (
split_command,should_use_cmd_mox_stub,normalise_cmd_mox_command) for use in tests and external modules. The__all__declaration appropriately includes the_CommandRunnerprotocol for typing support alongside the new public exports.docs/lading-design.md (1)
185-210: LGTM: Data flow documentation clearly illustrates the refactored architecture.The new "Publish data flow" section provides a clear visual representation of how manifest and patch-stripping responsibilities are delegated across modules. The Mermaid diagram effectively communicates the decision points for
strip_patchesconfiguration and the interaction betweenpublish,publish_plan,publish_manifest, andpublish_executionmodules.tests/bdd/steps/test_common_steps.py (4)
13-13: LGTM: Centralised TOML utilities imported.
72-72: LGTM: Manifest loading delegated to centralised utility.
85-85: LGTM: Manifest loading delegated to centralised utility.
124-124: LGTM: Manifest loading delegated to centralised utility.tests/bdd/steps/test_publish_steps.py (4)
15-15: LGTM: Centralised TOML utilities imported.
160-164: LGTM: Minimal protocol supports cmd-mox integration.The
_CmdInvocationprotocol correctly defines the interface expected by cmd-mox handlers, exposing only the requiredargsfield.
351-351: LGTM: Manifest loading delegated to centralised utility.
151-157: The original review comment is incorrect; the import path is correct.The test correctly accesses
publish._normalise_cmd_mox_command. Thepublishmodule intentionally re-exports the publicnormalise_cmd_mox_commandfrompublish_executionas_normalise_cmd_mox_command(line 41 of publish.py). The test importspublishand uses that re-exported alias, which is the intended API surface. No changes are required.Likely an incorrect or invalid review comment.
tests/bdd/steps/config_fixtures.py (6)
64-83: LGTM: Configuration loading delegated to centralised utility.The refactor correctly uses
toml_utils.load_or_create_documentto handle both existing and missing configuration files, followed by defensive table/array construction before mutation. This pattern is consistently applied throughout the fixture.
145-151: LGTM: Configuration loading delegated to centralised utility.
159-168: LGTM: Configuration loading delegated to centralised utility.
179-189: LGTM: Configuration loading delegated to centralised utility.
200-207: LGTM: Configuration loading delegated to centralised utility.
237-240: LGTM: Configuration loading delegated to centralised utility.tests/bdd/toml_utils.py (1)
65-81: LGTM: Centralised manifest loaders provide consistent test interface.The three new loader functions (
load_manifest,load_workspace_manifest,load_crate_manifest) provide a clean, consistent interface for test code to load TOML manifests with helpful assertions on missing files. The delegation pattern (specialised loaders call the baseload_manifest) avoids duplication whilst maintaining clarity.Note:
load_crate_manifesthardcodes thecrates/directory structure, which matches the current workspace layout and test fixtures. This is acceptable for BDD test utilities that operate against controlled fixture workspaces.lading/commands/publish_manifest.py (1)
34-65: Keep the manifest load/write and strip-patch orchestration as-isRetain the current control flow:
_validate_and_load_manifestcorrectly short-circuits when stripping is disabled or no[patch.crates-io]exists,_apply_strategy_to_patchesenforces the"all" | "per-crate" | Falsecontract and fails fast for unsupported values, and_cleanup_empty_patch_tablesplus_write_manifest_documentensure clean TOML output only when modifications occur. This matches the described strip-patch semantics and provides clear failure modes viaPublishPreparationError.Also applies to: 80-139
lading/commands/publish.py (2)
15-21: Maintain helper aliasing to preserve the previous publish API surfaceKeep the new wiring that imports
normalise_cmd_mox_command,should_use_cmd_mox_stub, andsplit_commandfrompublish_executionand re-exports them via the underscored aliases, and similarly exposesappend_section,format_plan, andPublishPlanErrorfrompublish_plan. This preserves existing import paths (lading.commands.publish._split_command,PublishPlanError, etc.) while allowing the implementation to live in more focused modules.Also applies to: 22-25, 26-45
336-371: Retain the ordering of strip-patch application within the publishrunflowKeep
_apply_strip_patch_strategyinvoked immediately afterprepare_workspaceand before plan rendering, passingpreparation.staging_root, the computedplan, andactive_configuration.publish.strip_patches. This preserves the semantics that patch-stripping acts on the stagedCargo.tomlonly when staging is active and before any user-visible plan or subsequent steps rely on it, matching the stated behaviour for"all","per-crate", andFalse.
Add a pytest parametrized test to verify that preflight command expectations correctly normalize various cargo subcommands and arguments. This enhances test coverage for command normalization in the publish workflow. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
… publish manifest utilities - Updated type hints from Any to object for better type precision in patch-related mappings. - Expanded module docstring to include summary, function references, and examples. - Removed redundant existence checks in test fixtures related to manifest file loading. These improvements clarify the codebase and ensure proper documentation for publish manifest helpers. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
lading/commands/publish_manifest.py(1 hunks)lading/testing/__init__.py(1 hunks)lading/testing/toml_utils.py(3 hunks)tests/bdd/conftest.py(1 hunks)tests/bdd/steps/config_fixtures.py(4 hunks)tests/bdd/steps/manifest_fixtures.py(3 hunks)tests/bdd/steps/test_common_steps.py(4 hunks)tests/bdd/steps/test_publish_steps.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/bdd/steps/manifest_fixtures.pytests/bdd/steps/test_common_steps.pytests/bdd/conftest.pytests/bdd/steps/config_fixtures.pylading/testing/__init__.pylading/commands/publish_manifest.pytests/bdd/steps/test_publish_steps.pylading/testing/toml_utils.py
🧬 Code graph analysis (5)
tests/bdd/steps/manifest_fixtures.py (1)
lading/testing/toml_utils.py (1)
load_manifest(65-70)
tests/bdd/steps/test_common_steps.py (1)
lading/testing/toml_utils.py (1)
load_manifest(65-70)
tests/bdd/steps/config_fixtures.py (1)
lading/testing/toml_utils.py (1)
load_or_create_document(28-32)
tests/bdd/steps/test_publish_steps.py (1)
lading/testing/toml_utils.py (1)
load_manifest(65-70)
lading/testing/toml_utils.py (1)
crate_tools/unittests/test_publish_workspace_dependencies.py (1)
workspace_root(16-21)
🔍 Remote MCP Deepwiki, Ref
Summary of additional relevant facts for reviewing PR #42
-
Repository wiki contains a "publish Command" section with "Preflight Checks", "Publication Planning", and "Workspace Staging" pages that are likely relevant to where strip-patches and manifest staging belong — check these for expected behavior and call order (publish preflight → plan → workspace staging).
-
Attempts to search code/docs for concrete symbols (publish_manifest.py, _apply_strip_patch_strategy, PublishPreparationError, StripPatchesSetting) via the documentation search tool failed (HTTP 402). Retry or fetch the repository files directly (or read the new module) to verify:
- That _apply_strip_patch_strategy is invoked at the same publish staging point as before.
- That PublishPreparationError mappings and messages match prior expectations (tests expect specific error types/messages).
- That StripPatchesSetting enum/alias values ("all" | "per-crate" | false) are preserved and handled exactly as before.
Actionable checks to perform in code review (based on above):
- Confirm publish.run still calls _apply_strip_patch_strategy at the same stage and that behavior for the three strip_patches settings is identical.
- Verify PublishPreparationError is raised in the same failure scenarios and that tests catching it still apply.
- Verify re-exported names and all entries in publish_execution.py and publish_plan.py match all import sites in the repo and tests.
- Run tests (BDD) that cover manifest staging and cmd-mox normalization to ensure integration with new toml_utils and publish_manifest behaviors.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (24)
lading/testing/toml_utils.py (2)
17-25: LGTM!The
__all__list is correctly updated to expose the new manifest loading functions.
65-75: LGTM!The functions correctly validate manifest existence and provide helpful error messages. Type hints follow PEP 604 style, and delegation pattern in
load_workspace_manifestis appropriate.lading/testing/__init__.py (1)
1-3: LGTM!The package initialisation is minimal and appropriate. The module docstring adequately describes the package purpose.
tests/bdd/conftest.py (1)
1-8: LGTM!The conftest correctly imports step modules to register their pytest-bdd definitions. The
noqa: F401suppressions are appropriately used for side-effect imports, and the explanatory comment at line 5 provides clear justification.tests/bdd/steps/manifest_fixtures.py (3)
9-10: LGTM!The import correctly uses the centralized TOML utilities from
lading.testing, resolving the previous concern about importing test modules.
22-22: LGTM!The code correctly delegates manifest loading to
toml_utils.load_manifest, which handles existence checking internally. The redundant check mentioned in past reviews has been properly removed.
85-85: LGTM!Consistent use of the centralized manifest loading utility.
tests/bdd/steps/test_common_steps.py (2)
13-13: LGTM!The import correctly uses the centralized TOML utilities from
lading.testing, resolving the previous "dont-import-test-modules" concern.
68-68: LGTM!Consistent adoption of
toml_utils.load_manifestacross all manifest loading sites. The centralised approach improves maintainability and eliminates code duplication.Also applies to: 81-81, 120-120
tests/bdd/steps/config_fixtures.py (2)
11-11: LGTM!The import correctly uses the centralized TOML utilities from
lading.testing, resolving the previous "dont-import-test-modules" concern.
25-29: LGTM!Consistent adoption of
toml_utilshelpers (load_or_create_document,ensure_table,ensure_array_field,append_if_absent) throughout the fixtures eliminates code duplication and improves maintainability.Also applies to: 35-38, 64-83, 132-138, 145-151, 159-168, 179-189, 200-207, 237-240
tests/bdd/steps/test_publish_steps.py (5)
14-14: LGTM!The import correctly uses the centralized TOML utilities from
lading.testing, resolving the previous "dont-import-test-modules" concern.
147-153: LGTM!The function correctly delegates cargo command normalisation to
publish._normalise_cmd_mox_command. Accessing private functions in tests is acceptable for verifying internal behaviour, and this change aligns with the centralized command-handling approach described in the PR objectives.
156-161: LGTM!The
_CmdInvocationProtocol correctly defines the expected structure for cmd-mox invocation payloads. Type hints follow modern style withtyp.Sequence[str].
180-206: LGTM!The parametrized test comprehensively covers cargo command normalisation across common subcommands (
check,test,clippy,fmt,build,doc) and complex invocations with flags and argument separators. This addresses the past review comment about extending test coverage for cmd-mox normalisation edge cases.
375-375: LGTM!Correct usage of the centralized manifest loading utility.
lading/commands/publish_manifest.py (8)
1-29: Excellent module-level documentation!The expanded NumPy-style docstring now provides clear context on the module's purpose, call sites, and usage examples. This addresses the previous feedback and aligns with project documentation guidelines.
31-59: Solid type annotations and import handling!The imports use
TYPE_CHECKINGguards effectively, theTOMLDocumentimport includes proper defensive handling with coverage pragmas, and the type aliases follow modern PEP 604 syntax whilst usingobjectfor TOML values rather thanAny. This aligns well with the project's type safety guidelines.
66-80: Robust error handling for manifest loading!The function provides comprehensive exception handling for file access, permissions, and TOML parsing errors, with clear context in the raised
PublishPreparationErrormessages. The defensive guards are appropriately marked with coverage pragmas.
83-92: LGTM: proper trivia preservation and newline handling!The function correctly preserves TOML formatting via
document.as_string()and ensures a trailing newline for POSIX compliance. Error handling is appropriate.
95-105: Clever deduplication strategy!Using
dict.fromkeys()to deduplicate crate names whilst preserving order ensures deterministic updates. The logic is clean and the boolean return clearly signals modifications.
121-137: Clean validation logic with appropriate early returns!The function correctly handles the three-valued
StripPatchesSetting(False, "all", "per-crate") with an explicitis Falsecheck, and the final conditional expression cleanly handles the optional patch-table return. The NumPy-style docstring is fitting for this helper.
140-149: LGTM: straightforward cleanup logic!The function correctly removes empty nested tables in the proper order (innermost first), maintaining TOML document consistency.
167-190: Excellent orchestration of the patch-stripping workflow!The function clearly coordinates the validation, strategy application, cleanup, and write-back steps with appropriate early returns to minimise complexity. The logic flow matches the documented publish staging behaviour.
…ring with details and examples docs(testing,toml_utils): add comprehensive module docstring with summary, usage, and examples - Enhanced PublishPreparationError docstring to clarify error cases and provide usage example. - Enriched toml_utils module docstring to summarize purpose, demonstrate usage, and show example manifests. - Updated load_crate_manifest signature for improved flexibility and documented behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…ble handling Refactor _resolve_patch_tables and _apply_strategy_to_patches to use Python 3.10 structural pattern matching for improved readability and maintainability when handling patch tables in manifests. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
lading/commands/publish_manifest.py(1 hunks)lading/testing/toml_utils.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
lading/testing/toml_utils.pylading/commands/publish_manifest.py
🧬 Code graph analysis (1)
lading/testing/toml_utils.py (1)
crate_tools/unittests/test_publish_workspace_dependencies.py (1)
workspace_root(16-21)
🔍 Remote MCP Ref
Summary of additional concrete facts found (concise, review-focused)
-
New module added: lading/commands/publish_manifest.py — defines PublishPreparationError and implements manifest staging helpers: _load_manifest_document, _write_manifest_document, _remove_per_crate_entries, _resolve_patch_tables, _validate_and_load_manifest, _cleanup_empty_patch_tables, _apply_strategy_to_patches, and _apply_strip_patch_strategy. These functions handle TOML IO, parse errors, patch table resolution, per-crate deduplication, strategy application ("all" vs "per-crate"), cleanup of empty patch tables, and conditional write-back when staging is active.,
-
publish.py now delegates manifest-stage work to publish_manifest._apply_strip_patch_strategy (replacing prior in-file implementations) and re-exports plan/execution helpers (append_section, format_plan, split_command, should_use_cmd_mox_stub, normalise_cmd_mox_command) as aliases to maintain backward compatibility — verify all import sites still match these exported names.
-
Tests updated to use centralized TOML utilities in lading/testing/toml_utils.py (new helpers: load_manifest, load_workspace_manifest, load_crate_manifest) and multiple BDD fixtures/tests were changed to call these helpers instead of manual tomlkit parsing; ensure test fixtures still create/read staged Cargo.toml paths expected by publish_manifest.
-
Observability / error semantics to verify in review:
- PublishPreparationError is used for IO/parsing/staging failures — confirm tests expecting this exception still match messages/conditions.
- _apply_strip_patch_strategy only writes when modifications occurred and staging setting is active — confirm behavior matches previous semantics for StripPatchesSetting values ("all", "per-crate", false).
Files/locations to inspect closely in code review
- lading/commands/publish_manifest.py (new implementation & error messages) — confirm exception types/messages and write semantics.
- lading/commands/publish.py — call sites to _apply_strip_patch_strategy and any changed exported names.
- lading/commands/publish_execution.py and lading/commands/publish_plan.py — ensure all and aliases match imports across repo/tests.
- lading/testing/toml_utils.py and tests/bdd/* (manifest_fixtures.py, config_fixtures.py, test_publish_steps.py, test_common_steps.py) — confirm test helpers and fixtures use the new load_manifest paths and that staged manifest locations match publish_manifest expectations.
Caveat
- Some automated documentation searches/read attempts returned transient failures; concrete file content for publish_manifest.py and several search hits were read/queried but may require re-check in the PR branch to validate message strings and exact control-flow edge cases.,
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (10)
lading/testing/toml_utils.py (2)
1-21: Excellent module docstring.The expanded docstring clearly explains purpose, utility, and usage with concrete examples. The en-GB spelling and structure align perfectly with the coding guidelines.
37-45: Public API exports are correct.The all declaration properly includes the three new manifest-loading helpers, maintaining alphabetical ordering.
lading/commands/publish_manifest.py (8)
51-59: LGTM!The type aliases use modern PEP 695 syntax and correctly use
objectinstead ofAnyfor opaque TOML values, aligning with past review feedback.
62-79: LGTM!The exception class now has a comprehensive NumPy-style docstring with clear "Raised when" scenarios and an example, addressing prior feedback.
82-96: LGTM!Robust error handling with proper exception chaining and clear error messages. The single-line docstring is appropriate for a private function.
99-108: LGTM!The function correctly ensures a trailing newline for POSIX compliance and handles write failures defensively.
111-121: LGTM!The deduplication via
dict.fromkeysis a clean idiom, and the comment clarifies intent. The modification tracking is correct.
139-167: LGTM!The early-return logic in
_validate_and_load_manifestis clear, and theis Falsecheck is correct for aLiteral[False]type. The cleanup function correctly handles cascading empty-table removal.
170-210: LGTM!The strategy dispatch uses pattern matching as per project guidelines, and the orchestration function correctly avoids unnecessary writes by tracking modifications. The workflow is clear and maintainable.
124-136: Pattern matching with ABC classes in_resolve_patch_tablesis correct and functional.Verification confirms the structural pattern matching implementation works as intended:
- Python 3.13+ (project requirement) fully supports pattern matching with ABC class patterns
- Testing with
dictand customMutableMappingsubclasses confirms the pattern{"patch": cabc.MutableMapping() as patch_table}works correctly and matches nested structurestomlkit'sTOMLDocumentimplements theMutableMappingprotocol, so both the outer and inner pattern matches will function correctly at runtime- The function is actively used in the codebase (called at line 154 of
_validate_and_load_manifest) and returns the expected tuple typeNo issues found. The code correctly returns the patch and crates-io tables when both are present in the document, or
Noneotherwise.
… functions Add comprehensive docstrings for load_manifest, load_workspace_manifest, and load_crate_manifest functions in testing.toml_utils.py. These docstrings describe parameters, return types, and raised exceptions to improve code clarity and maintainability. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
lading/commands/publish_manifest.py(1 hunks)lading/testing/toml_utils.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
lading/testing/toml_utils.pylading/commands/publish_manifest.py
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (1)
lading/commands/publish_manifest.py (1)
1-55: Keep module structure, typing, and pattern matching as-isRetain the overall design in this module. The module-level docstring, use of
StripPatchesSetting, structural pattern matching in_resolve_patch_tablesand_apply_strategy_to_patches, and the staging/write-back helpers all align cleanly with the stated publish workflow and project guidelines.Also applies to: 78-107, 120-152, 166-206
…ctions Enhance documentation in toml_utils.py by providing comprehensive and clear docstrings for functions dealing with TOML document loading, table and array handling, and appending values. This improves code maintainability and usability by clarifying expected parameters, return types, and possible exceptions. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
1 participant
Summary
Changes
Why this change
Testing plan
Additional notes
📎 Task: https://www.terragonlabs.com/task/a269ba6e-8c81-4735-a41b-579ad2623bf9