Bring oversized files under the 400-line guideline (#108) - #136
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughThe PR extracts oversized modules into dedicated components: shared TOML coercion helpers ( ChangesModule extraction and wiring
Sequence Diagram(s)sequenceDiagram
participant load_workspace
participant build_workspace_graph
participant cargo_metadata
participant CargoToml
load_workspace->>cargo_metadata: load_cargo_metadata()
cargo_metadata-->>build_workspace_graph: metadata
build_workspace_graph->>CargoToml: inspect package.readme.workspace
CargoToml-->>build_workspace_graph: manifest data
build_workspace_graph-->>load_workspace: WorkspaceGraph
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (17 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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. Include the file and symbol names indicated in the issue at the head of your response. lading/commands/publish_pipeline.py Comment on lines +109 to +123 def _package_publishable_crates(
plan: PublishPlan,
preparation: PublishPreparation,
*,
options: _PublishExecutionOptions,
runner: CommandRunner,
) -> None:
"""Package each publishable crate in order using the staged workspace."""
state = _PublicationPipelineState(plan, preparation, options)
for crate in plan.publishable:
_package_crate(
crate,
state,
runner=runner,
)❌ New issue: Code Duplication |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on file """Shared TOML scalar, sequence, and mapping coercion helpers.❌ New issue: Global Conditionals |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. tests/unit/test_bump_command_internals.py Comment on file changes, "1.2.3", dry_run=False, workspace_root=root
)
if not (manifests or documents or readmes or lockfiles):❌ New issue: Complex Conditional |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. tests/unit/test_bump_command_internals.py Comment on file import dataclasses as dc
import typing as typ
from pathlib import Path❌ New issue: Low Cohesion |
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 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.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. tests/unit/test_bump_context_and_output.py Comment on lines +216 to +267 def test_result_message_grammar_and_path_rendering(
manifest_count: int,
document_count: int,
readme_count: int,
lockfile_count: int,
) -> None:
"""Any category combination renders correct grammar and unique paths."""
root = Path("/ws")
manifests = tuple(root / f"m{i}" / "Cargo.toml" for i in range(manifest_count))
documents = tuple(root / f"doc{i}.md" for i in range(document_count))
readmes = tuple(root / f"r{i}" / "README.md" for i in range(readme_count))
lockfiles = tuple(root / f"l{i}" / "Cargo.lock" for i in range(lockfile_count))
changes = bump_output.BumpChanges(
manifests=manifests,
documents=documents,
lockfiles=lockfiles,
transposed_readmes=readmes,
)
message = bump_output._format_result_message(
changes, "1.2.3", dry_run=False, workspace_root=root
)
if not (manifests or documents or readmes or lockfiles):
assert message == "No manifest changes required; all versions already 1.2.3."
return
lines = message.splitlines()
expected_body = [
*(f"- {path.relative_to(root)}" for path in manifests),
*(f"- {path.relative_to(root)} (documentation)" for path in documents),
*(f"- {path.relative_to(root)} (readme)" for path in readmes),
*(f"- {path.relative_to(root)} (lockfile)" for path in lockfiles),
]
assert lines[1:] == expected_body
categories = []
if manifests:
categories.append(f"{len(manifests)} manifest(s)")
if documents:
categories.append(f"{len(documents)} documentation file(s)")
if readmes:
categories.append(f"{len(readmes)} readme file(s)")
if lockfiles:
categories.append(f"{len(lockfiles)} lockfile(s)")
expected_header = (
f"Updated version to 1.2.3 in {_expected_description(categories)}:"
)
assert lines[0] == expected_header
assert " and and " not in lines[0]
if len(categories) >= 3:
assert ", and " in lines[0]❌ New issue: Complex Method |
This comment was marked as resolved.
This comment was marked as resolved.
7da9557 to
22b4f4e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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. Include the file and symbol names indicated in the issue at the head of your response. Complex Conditionaltests/unit/test_bump_context_and_output.py: test_result_message_grammar_and_path_rendering What lead to degradation?test_result_message_grammar_and_path_rendering has 1 complex conditionals with 3 branches, threshold = 2 Why does this problem occur?A complex conditional is an expression inside a branch such as an if-statmeent which consists of multiple, logical operations. Example: if (x.started() && y.running()).Complex conditionals make the code even harder to read, and contribute to the Complex Method code smell. Encapsulate them. How to fix it?Apply the DECOMPOSE CONDITIONAL refactoring so that the complex conditional is encapsulated in a separate function with a good name that captures the business rule. Optionally, for simple expressions, introduce a new variable which holds the result of the complex conditional. 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_conditional.js
function messageReceived(message, timeReceived) {
- // Ignore all messages which aren't from known customers:
- if (!message.sender &&
- customers.getId(message.name) == null) {
+ // Refactoring #1: encapsulate the business rule in a
+ // function. A clear name replaces the need for the comment:
+ if (!knownCustomer(message)) {
log('spam received -- ignoring');
return;
}
- // Provide an auto-reply when outside business hours:
- if ((timeReceived.getHours() > 17) ||
- (timeReceived.getHours() < 8)) {
+ // Refactoring #2: encapsulate the business rule.
+ // Again, note how a clear function name replaces the
+ // need for a code comment:
+ if (outsideBusinessHours(timeReceived)) {
return autoReplyTo(message);
}
pingAgentFor(message);
+}
+
+function outsideBusinessHours(timeReceived) {
+ // Refactoring #3: replace magic numbers with
+ // symbols that communicate with the code reader:
+ const closingHour = 17;
+ const openingHour = 8;
+
+ const hours = timeReceived.getHours();
+
+ // Refactoring #4: simple conditional rules can
+ // be further clarified by introducing a variable:
+ const afterClosing = hours > closingHour;
+ const beforeOpening = hours < openingHour;
+
+ // Yeah -- look how clear the business rule is now!
+ return afterClosing || beforeOpening;
} |
This comment was marked as resolved.
This comment was marked as resolved.
978b446 to
989256e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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. Include the file and symbol names indicated in the issue at the head of your response. tests/unit/test_bump_context_and_output.py Comment on lines +216 to +268 def test_result_message_grammar_and_path_rendering(
manifest_count: int,
document_count: int,
readme_count: int,
lockfile_count: int,
) -> None:
"""Any category combination renders correct grammar and unique paths."""
root = Path("/ws")
manifests = tuple(root / f"m{i}" / "Cargo.toml" for i in range(manifest_count))
documents = tuple(root / f"doc{i}.md" for i in range(document_count))
readmes = tuple(root / f"r{i}" / "README.md" for i in range(readme_count))
lockfiles = tuple(root / f"l{i}" / "Cargo.lock" for i in range(lockfile_count))
changes = bump_output.BumpChanges(
manifests=manifests,
documents=documents,
lockfiles=lockfiles,
transposed_readmes=readmes,
)
message = bump_output._format_result_message(
changes, "1.2.3", dry_run=False, workspace_root=root
)
has_no_changes = not any((manifests, documents, readmes, lockfiles))
if has_no_changes:
assert message == "No manifest changes required; all versions already 1.2.3."
return
lines = message.splitlines()
expected_body = [
*(f"- {path.relative_to(root)}" for path in manifests),
*(f"- {path.relative_to(root)} (documentation)" for path in documents),
*(f"- {path.relative_to(root)} (readme)" for path in readmes),
*(f"- {path.relative_to(root)} (lockfile)" for path in lockfiles),
]
assert lines[1:] == expected_body
categories = []
if manifests:
categories.append(f"{len(manifests)} manifest(s)")
if documents:
categories.append(f"{len(documents)} documentation file(s)")
if readmes:
categories.append(f"{len(readmes)} readme file(s)")
if lockfiles:
categories.append(f"{len(lockfiles)} lockfile(s)")
expected_header = (
f"Updated version to 1.2.3 in {_expected_description(categories)}:"
)
assert lines[0] == expected_header
assert " and and " not in lines[0]
if len(categories) >= 3:
assert ", and " in lines[0]❌ New issue: Complex Method |
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.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. tests/unit/test_bump_context_and_output.py Comment on lines +199 to +212 def _build_test_change_body(
root: Path,
manifests: tuple[Path, ...],
documents: tuple[Path, ...],
readmes: tuple[Path, ...],
lockfiles: tuple[Path, ...],
) -> list[str]:
"""Build the expected body lines for a result-message assertion."""
return [
*(f"- {path.relative_to(root)}" for path in manifests),
*(f"- {path.relative_to(root)} (documentation)" for path in documents),
*(f"- {path.relative_to(root)} (readme)" for path in readmes),
*(f"- {path.relative_to(root)} (lockfile)" for path in lockfiles),
]❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…actors main independently landed this branch's bundled #95/#96/#97, refactored bump.py (new `_CrateManifestOutcome` flow) and publish.py (#73/#96), and re-split the bump tests into differently-named modules. This reconciles the #108 extraction onto main's current tree as a single squashed change (the original 22 commits are preserved on backup/issue-108-pre-rebase2-7df1480). Landed cleanly (all now under the 400-line guideline): - `lading/toml_coerce/` package (scalar/sequence/mapping coercers with structural pattern matching and numpy docstrings) + `config.py` (441->334) and `workspace/models.py` (576->194) consuming it via `functools.partial`. - `lading/workspace/_coercion.py` + `graph_build.py` (builders out of `models`); `cli_options.py` (cyclopts args out of `cli.py`, 449->389); `bump_docs.py` condensed (402->393). - `bump_manifests.py` re-derived against main's bump.py: the low-level manifest helpers, `_WORKSPACE_SELECTORS`, and `_BumpContext` move here; main's `_apply_crate_manifest_update`/`_CrateManifestOutcome` flow stays in `bump` and imports them. `bump.py` 552->427 (further trim is follow-up). Test reconciliation: main's bump-test split is authoritative; this drops the branch's parallel split modules and keeps main's plus the unique `test_toml_coerce.py`. Per the no-compat-alias convention (#164), `test_dependency_sections` now reads `_DEPENDENCY_SECTION_BY_KIND` from `bump_manifests`. Deferred: `publish.py` (741) extraction. main's #73/#96 refactored the extracted pipeline functions and its tests patch `publish._*` seams; extracting now requires rewriting many `monkeypatch.setattr` targets to `publish_pipeline` (no-compat), which needs a focused full-context pass. publish.py is taken from main unchanged; the orphaned `publish_pipeline.py` is removed. Validated: check-fmt, lint (10.00/10), typecheck, test (717 passed, 70 snapshots), markdownlint, nixie — all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7df1480 to
e53d6e1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lading/toml_coerce/_sequences.py (1)
102-141: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
string_matrix's hand-built message also skips_reject.
_validate_matrix_entrycorrectly raises via_rejectat every branch, butstring_matrixbuilds its ownmessagestring and callserror(message)directly, dropping the"; received {type}."suffix that the canonical shape requires. This is a pre-existing pattern (the value it produces is pinned by thetest_coercion_error_messages_are_stablesnapshot forpreflight.aux_build), so fix it deliberately alongside a snapshot regeneration rather than leaving it as the odd one out in an otherwise-uniform module.♻️ Proposed fix (requires regenerating the syrupy snapshot)
def string_matrix( value: object, field_name: str, *, error: _ErrorType ) -> tuple[tuple[str, ...], ...]: """Return a tuple-of-tuples parsed from ``value`` as string sequences.""" - message = f"{field_name} must be a sequence of string sequences." match value: case None: return () case str() | bytes(): - raise error(message) + raise _reject(value, field_name, "a sequence of string sequences", error) case cabc.Sequence(): return tuple( _validate_matrix_entry(entry, field_name, index, error) for index, entry in enumerate(value) ) case _: - raise error(message) + raise _reject(value, field_name, "a sequence of string sequences", error)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lading/toml_coerce/_sequences.py` around lines 102 - 141, The string_matrix function is bypassing the standard _reject-based error shape by raising error(message) directly, so its failures miss the canonical "; received {type}." suffix. Update string_matrix to use the same _reject pattern as _validate_matrix_entry, keeping the error wording consistent with the module’s other coercion helpers, and then regenerate the affected snapshot/test expectation for test_coercion_error_messages_are_stable (preflight.aux_build).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/users-guide.md`:
- Around line 295-301: The fenced text example in the users guide exceeds the
120-column code-block wrap limit. Reformat the aggregated-error example in the
markdown snippet so the long “Cargo lockfile regeneration failed...” sentence is
split across multiple lines while preserving the existing meaning and the `cargo
update --workspace --manifest-path ...` lines; use the example block in
`docs/users-guide.md` to locate it, then run `make fmt` to verify the
markdownlint-friendly wrapping.
In `@lading/commands/bump_manifests.py`:
- Around line 46-48: The `_BumpContext` docstring contains a stale Sphinx
cross-reference that points to `_update_crate_manifest`, which no longer exists.
Update the reference in `_BumpContext` to use
`bump._apply_crate_manifest_update` so the documentation resolves correctly and
matches the current consumer of the context.
In `@lading/toml_coerce/_scalars.py`:
- Around line 99-120: non_negative_int is duplicating the canonical
error-message format instead of using the centralized helper. Update the
type-check and parse-failure paths in non_negative_int to call _reject with the
field name and expected type information, and use the same helper for the
negative-value case if that is also intended to share the canonical wording;
keep the existing control flow and identifiers like non_negative_int and _reject
so _core.py remains the single source of truth.
In `@lading/toml_coerce/_sequences.py`:
- Around line 35-54: The disallowed-None branch in expect_sequence uses a
one-off error message instead of the module’s shared rejection shape. Update the
None case in expect_sequence to route through _reject, matching the same
"{field_name} must be ...; received {type(x).__name__}." pattern used by the
string/bytes and fallback branches. Keep allow_none returning None unchanged,
and preserve the existing expected wording for a sequence.
In `@lading/workspace/graph_build.py`:
- Around line 1-8: Update the module docstring in graph_build.py so it no longer
says the coercion bindings are defined in models; change that reference to
lading.workspace._coercion to match the current import source. Keep the rest of
the description intact and make sure the wording still points maintainers to the
correct place for coercion-related validation behavior in this module.
---
Outside diff comments:
In `@lading/toml_coerce/_sequences.py`:
- Around line 102-141: The string_matrix function is bypassing the standard
_reject-based error shape by raising error(message) directly, so its failures
miss the canonical "; received {type}." suffix. Update string_matrix to use the
same _reject pattern as _validate_matrix_entry, keeping the error wording
consistent with the module’s other coercion helpers, and then regenerate the
affected snapshot/test expectation for test_coercion_error_messages_are_stable
(preflight.aux_build).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bd0304ff-1a83-4fcb-9b78-8579aaaf77e3
📒 Files selected for processing (23)
docs/developers-guide.mddocs/users-guide.mdlading/cli.pylading/cli_options.pylading/commands/bump.pylading/commands/bump_docs.pylading/commands/bump_manifests.pylading/config.pylading/toml_coerce/__init__.pylading/toml_coerce/_core.pylading/toml_coerce/_mappings.pylading/toml_coerce/_scalars.pylading/toml_coerce/_sequences.pylading/workspace/__init__.pylading/workspace/_coercion.pylading/workspace/graph_build.pylading/workspace/models.pytests/bdd/steps/test_bump_steps.pytests/unit/__snapshots__/test_toml_coerce.ambrtests/unit/test_cli.pytests/unit/test_dependency_sections.pytests/unit/test_toml_coerce.pytests/unit/test_workspace_models_validation.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/shared-actions(auto-detected)
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lading/toml_coerce/_sequences.py (1)
102-141: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
string_matrix's hand-built message also skips_reject.
_validate_matrix_entrycorrectly raises via_rejectat every branch, butstring_matrixbuilds its ownmessagestring and callserror(message)directly, dropping the"; received {type}."suffix that the canonical shape requires. This is a pre-existing pattern (the value it produces is pinned by thetest_coercion_error_messages_are_stablesnapshot forpreflight.aux_build), so fix it deliberately alongside a snapshot regeneration rather than leaving it as the odd one out in an otherwise-uniform module.♻️ Proposed fix (requires regenerating the syrupy snapshot)
def string_matrix( value: object, field_name: str, *, error: _ErrorType ) -> tuple[tuple[str, ...], ...]: """Return a tuple-of-tuples parsed from ``value`` as string sequences.""" - message = f"{field_name} must be a sequence of string sequences." match value: case None: return () case str() | bytes(): - raise error(message) + raise _reject(value, field_name, "a sequence of string sequences", error) case cabc.Sequence(): return tuple( _validate_matrix_entry(entry, field_name, index, error) for index, entry in enumerate(value) ) case _: - raise error(message) + raise _reject(value, field_name, "a sequence of string sequences", error)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lading/toml_coerce/_sequences.py` around lines 102 - 141, The string_matrix function is bypassing the standard _reject-based error shape by raising error(message) directly, so its failures miss the canonical "; received {type}." suffix. Update string_matrix to use the same _reject pattern as _validate_matrix_entry, keeping the error wording consistent with the module’s other coercion helpers, and then regenerate the affected snapshot/test expectation for test_coercion_error_messages_are_stable (preflight.aux_build).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/users-guide.md`:
- Around line 295-301: The fenced text example in the users guide exceeds the
120-column code-block wrap limit. Reformat the aggregated-error example in the
markdown snippet so the long “Cargo lockfile regeneration failed...” sentence is
split across multiple lines while preserving the existing meaning and the `cargo
update --workspace --manifest-path ...` lines; use the example block in
`docs/users-guide.md` to locate it, then run `make fmt` to verify the
markdownlint-friendly wrapping.
In `@lading/commands/bump_manifests.py`:
- Around line 46-48: The `_BumpContext` docstring contains a stale Sphinx
cross-reference that points to `_update_crate_manifest`, which no longer exists.
Update the reference in `_BumpContext` to use
`bump._apply_crate_manifest_update` so the documentation resolves correctly and
matches the current consumer of the context.
In `@lading/toml_coerce/_scalars.py`:
- Around line 99-120: non_negative_int is duplicating the canonical
error-message format instead of using the centralized helper. Update the
type-check and parse-failure paths in non_negative_int to call _reject with the
field name and expected type information, and use the same helper for the
negative-value case if that is also intended to share the canonical wording;
keep the existing control flow and identifiers like non_negative_int and _reject
so _core.py remains the single source of truth.
In `@lading/toml_coerce/_sequences.py`:
- Around line 35-54: The disallowed-None branch in expect_sequence uses a
one-off error message instead of the module’s shared rejection shape. Update the
None case in expect_sequence to route through _reject, matching the same
"{field_name} must be ...; received {type(x).__name__}." pattern used by the
string/bytes and fallback branches. Keep allow_none returning None unchanged,
and preserve the existing expected wording for a sequence.
In `@lading/workspace/graph_build.py`:
- Around line 1-8: Update the module docstring in graph_build.py so it no longer
says the coercion bindings are defined in models; change that reference to
lading.workspace._coercion to match the current import source. Keep the rest of
the description intact and make sure the wording still points maintainers to the
correct place for coercion-related validation behavior in this module.
---
Outside diff comments:
In `@lading/toml_coerce/_sequences.py`:
- Around line 102-141: The string_matrix function is bypassing the standard
_reject-based error shape by raising error(message) directly, so its failures
miss the canonical "; received {type}." suffix. Update string_matrix to use the
same _reject pattern as _validate_matrix_entry, keeping the error wording
consistent with the module’s other coercion helpers, and then regenerate the
affected snapshot/test expectation for test_coercion_error_messages_are_stable
(preflight.aux_build).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bd0304ff-1a83-4fcb-9b78-8579aaaf77e3
📒 Files selected for processing (23)
docs/developers-guide.mddocs/users-guide.mdlading/cli.pylading/cli_options.pylading/commands/bump.pylading/commands/bump_docs.pylading/commands/bump_manifests.pylading/config.pylading/toml_coerce/__init__.pylading/toml_coerce/_core.pylading/toml_coerce/_mappings.pylading/toml_coerce/_scalars.pylading/toml_coerce/_sequences.pylading/workspace/__init__.pylading/workspace/_coercion.pylading/workspace/graph_build.pylading/workspace/models.pytests/bdd/steps/test_bump_steps.pytests/unit/__snapshots__/test_toml_coerce.ambrtests/unit/test_cli.pytests/unit/test_dependency_sections.pytests/unit/test_toml_coerce.pytests/unit/test_workspace_models_validation.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/shared-actions(auto-detected)
🛑 Comments failed to post (5)
docs/users-guide.md (1)
295-301: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the aggregated-error example past the 120-column code limit.
Line 296 runs to roughly 166 characters — break it across lines so the fenced
textblock stays inside the documented 120-column wrap for code blocks.✏️ Proposed fix
```text -Cargo lockfile regeneration failed for 2 manifest(s). Manifests already carry the new version, so the workspace is inconsistent until each lockfile below is repaired: +Cargo lockfile regeneration failed for 2 manifest(s). Manifests already carry +the new version, so the workspace is inconsistent until each lockfile below +is repaired: - Cargo lockfile regeneration failed for crates/a/Cargo.toml with exit code 101: <cargo error> cargo update --workspace --manifest-path crates/a/Cargo.toml - Cargo lockfile regeneration failed for crates/b/Cargo.toml with exit code 101: <cargo error> cargo update --workspace --manifest-path crates/b/Cargo.toml</details> Run `make fmt` afterwards to confirm the fix satisfies `make markdownlint`. Triage: `[type:syntax/md]` <!-- suggestion_start --> <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/users-guide.md` around lines 295 - 301, The fenced text example in the users guide exceeds the 120-column code-block wrap limit. Reformat the aggregated-error example in the markdown snippet so the long “Cargo lockfile regeneration failed...” sentence is split across multiple lines while preserving the existing meaning and the `cargo update --workspace --manifest-path ...` lines; use the example block in `docs/users-guide.md` to locate it, then run `make fmt` to verify the markdownlint-friendly wrapping.Sources: Coding guidelines, Path instructions
lading/commands/bump_manifests.py (1)
46-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the stale
:func:cross-reference in_BumpContext.The docstring points at
_update_crate_manifest, which no longer exists after the extraction. The context is now consumed bybump._apply_crate_manifest_update, so the Sphinx cross-reference resolves to nothing.✏️ Proposed fix
- Derived once by ``bump._initialize_bump_context`` and consumed by - :func:`_update_crate_manifest`; the manifest-mutation contract lives with - this extracted module rather than reaching back into ``bump`` internals. + Derived once by ``bump._initialize_bump_context`` and consumed by + ``bump._apply_crate_manifest_update``; the manifest-mutation contract lives + with this extracted module rather than reaching back into ``bump`` internals.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Derived once by ``bump._initialize_bump_context`` and consumed by ``bump._apply_crate_manifest_update``; the manifest-mutation contract lives with this extracted module rather than reaching back into ``bump`` internals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lading/commands/bump_manifests.py` around lines 46 - 48, The `_BumpContext` docstring contains a stale Sphinx cross-reference that points to `_update_crate_manifest`, which no longer exists. Update the reference in `_BumpContext` to use `bump._apply_crate_manifest_update` so the documentation resolves correctly and matches the current consumer of the context.lading/toml_coerce/_scalars.py (1)
99-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Reuse
_rejectinstead of hand-rolling the canonical message.
non_negative_intbuildstype_errormanually rather than calling_reject, duplicating the exact message shape that_core.pyexists to centralize. Route all three raise sites through_rejectso this module stays a single source of truth for the canonical format.♻️ Proposed fix
- type_error = f"{field_name} must be an integer; received {type(value).__name__}." - # ``bool`` is a subclass of ``int`` and ``float``/other types are truthy for - # a blanket ``int(...)`` cast, so dispatch explicitly to accept only real - # integers and integer-valued strings (the config string path). + # ``bool`` is a subclass of ``int`` and ``float``/other types are truthy for + # a blanket ``int(...)`` cast, so dispatch explicitly to accept only real + # integers and integer-valued strings (the config string path). match value: case None: return default case bool(): - raise error(type_error) + raise _reject(value, field_name, "an integer", error) case int(): integer = value case str(): try: integer = int(value) except ValueError as exc: - raise error(type_error) from exc + raise _reject(value, field_name, "an integer", error) from exc case _: - raise error(type_error) + raise _reject(value, field_name, "an integer", error)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# ``bool`` is a subclass of ``int`` and ``float``/other types are truthy for # a blanket ``int(...)`` cast, so dispatch explicitly to accept only real # integers and integer-valued strings (the config string path). match value: case None: return default case bool(): raise _reject(value, field_name, "an integer", error) case int(): integer = value case str(): try: integer = int(value) except ValueError as exc: raise _reject(value, field_name, "an integer", error) from exc case _: raise _reject(value, field_name, "an integer", error) if integer < 0: message = f"{field_name} must be non-negative." raise error(message) return integer🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lading/toml_coerce/_scalars.py` around lines 99 - 120, non_negative_int is duplicating the canonical error-message format instead of using the centralized helper. Update the type-check and parse-failure paths in non_negative_int to call _reject with the field name and expected type information, and use the same helper for the negative-value case if that is also intended to share the canonical wording; keep the existing control flow and identifiers like non_negative_int and _reject so _core.py remains the single source of truth.lading/toml_coerce/_sequences.py (1)
35-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Route the disallowed-
Nonebranch through_rejectfor a consistent error shape.The
None-not-allowed branch raiseserror(message)directly withmessage = f"{field_name} must be a sequence"— no"; received {type}."suffix. Every other rejection path in this module (and_rejectitself) follows"{field} must be {expected}; received {type(x).__name__}."— this one line breaks that contract without a documented reason.🐛 Proposed fix for the canonical message shape
match value: case None: if allow_none: return None - message = f"{field_name} must be a sequence" - raise error(message) + raise _reject(value, field_name, "a sequence", error)Based on the PR objective to "Keep the shared error-message shape:
{field} must be … received {type(x).__name__}", this branch should follow the same pattern as the rest of the module.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.match value: case None: if allow_none: return None raise _reject(value, field_name, "a sequence", error) case str() | bytes() | bytearray(): raise _reject(value, field_name, "a sequence", error) case cabc.Sequence(): return value case _: raise _reject(value, field_name, "a sequence", error)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lading/toml_coerce/_sequences.py` around lines 35 - 54, The disallowed-None branch in expect_sequence uses a one-off error message instead of the module’s shared rejection shape. Update the None case in expect_sequence to route through _reject, matching the same "{field_name} must be ...; received {type(x).__name__}." pattern used by the string/bytes and fallback branches. Keep allow_none returning None unchanged, and preserve the existing expected wording for a sequence.lading/workspace/graph_build.py (1)
1-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the stale "coercion bindings defined in
models" claim.The bindings moved to
lading.workspace._coercion(confirmed by the import block at line 19), yet the module docstring still says they're "defined inmodels". Update the reference so a maintainer debugging a coercion failure looks in the right module.✏️ Proposed fix
Extracted from :mod:`lading.workspace.models` (issue `#108`) so the data structures and topology stay separate from the metadata-parsing layer. The -module shares the coercion bindings defined in ``models`` so all workspace +module shares the coercion bindings defined in :mod:`lading.workspace._coercion` +so all workspace validation failures raise :class:`WorkspaceModelError` with the canonical message shape from :mod:`lading.toml_coerce`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."""Builders converting ``cargo metadata`` output into workspace models. Extracted from :mod:`lading.workspace.models` (issue `#108`) so the data structures and topology stay separate from the metadata-parsing layer. The module shares the coercion bindings defined in :mod:`lading.workspace._coercion` so all workspace validation failures raise :class:`WorkspaceModelError` with the canonical message shape from :mod:`lading.toml_coerce`. """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lading/workspace/graph_build.py` around lines 1 - 8, Update the module docstring in graph_build.py so it no longer says the coercion bindings are defined in models; change that reference to lading.workspace._coercion to match the current import source. Keep the rest of the description intact and make sure the wording still points maintainers to the correct place for coercion-related validation behavior in this module.
Centralise the canonical error-message shape in the toml_coerce helpers:
- `non_negative_int` type/parse-failure arms now raise
`_reject(value, field_name, "an integer", error)` (identical message, so the
snapshot is unchanged); the `str` arm preserves `from exc` chaining. The
negative-value message stays a plain value error (the `"; received {type}."`
type-shape does not fit it).
- `expect_sequence`'s None-disallowed branch and both `string_matrix` failure
arms now route through `_reject`, so they carry the canonical
`"; received {type}."` suffix. Regenerated the `preflight.aux_build` line of
the `test_coercion_error_messages_are_stable` snapshot accordingly.
Docstrings: `_BumpContext` now points at `bump._apply_crate_manifest_update`
(the current consumer; `_update_crate_manifest` was removed), and
`graph_build`'s module docstring points coercion bindings at
`lading.workspace._coercion` rather than `models`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Address the "canonical-message branches unguarded" review finding by asserting exact error wording (not just the raised type) on the branches that recently moved to `_reject`, and by covering the `expect_mapping` success path: - `test_expect_sequence_handles_none` now asserts the disallowed-None message `"f must be a sequence; received NoneType."`. - `test_string_matrix_rejects_non_sequence_values` asserts the top-level `"... must be a sequence of string sequences; received <type>."` shape. - `test_string_matrix_rejects_non_string_rows` asserts the row-level `"demo.matrix[1] must be a sequence of strings; received <type>."` shape. - `test_mapping_helpers_reject_non_mappings` becomes `test_mapping_helpers_accept_and_reject_mappings`, adding a passthrough assertion that a valid mapping is returned unchanged. Tests are strengthened in place (no new methods) to stay within the `TestTomlCoerce` PLR0904 public-method cap. The graph_build module-docstring finding was already fixed in the prior commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai please create a new GitHub issue for the following: Skipped — Architectural Complexity (warning): getting |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. tests/unit/test_toml_coerce.py Comment on lines +99 to +111 def test_string_tuple_rejects_non_string_entries(
self,
values: list[str],
bad_index: int,
bad: object,
error: type[Exception],
) -> None:
"""A non-string entry is rejected with its index in the field name."""
_assert_rejects_indexed_non_string(
toml_coerce.string_tuple,
"demo.list",
_IndexedRejectionCase(values, bad_index, bad, error),
)❌ New issue: Code Duplication |
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. Inline comments: In
In
In
In
Outside diff comments:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="leynos/lading"
body=$(cat <<'EOF'
## Summary
Follow-up from PR `#136` (issue `#108`): two source files still exceed the project's 400-line guideline and require further extraction beyond what was completed in that PR.
## Affected files
- `lading/commands/bump.py` — still ~27 lines over the 400-line limit. Docstring trimming alone cannot close the gap; further logic extraction is required (in the spirit of the `bump_manifests.py` split already landed).
- `lading/commands/publish.py` — 741 lines, deliberately deferred from PR `#136`.
## Rationale for deferral
Extracting `publish.py` is a substantial, non-minimal task because it requires:
1. Reconciling main's `#73/`#96 refactor with the pipeline extraction done in PR `#136` (`lading/commands/publish_pipeline.py`).
2. Rewriting the many `monkeypatch.setattr(publish, …)` test seams to target `publish_pipeline` instead, per the project's no-compat convention (no shim/back-compat layers).
This was judged unsuited to the review-fix pass on PR `#136` and tracked here as the agreed follow-up.
## Acceptance criteria
- [ ] `lading/commands/bump.py` is at or below 400 lines through further logic extraction (not just docstring trimming).
- [ ] `lading/commands/publish.py` is at or below 400 lines, with responsibilities distributed to `publish_pipeline.py` and/or other new modules as appropriate.
- [ ] All `monkeypatch.setattr(publish, …)` test seams are updated to target the correct module post-extraction, with no compatibility shims left behind.
- [ ] `docs/developers-guide.md` extraction map is updated to reflect the new module boundaries.
- [ ] `make lint`, `make typecheck`, and `make test` pass.
## References
- PR: https://github.com/leynos/lading/pull/136
- Requested by: `@leynos`
EOF
)
gh issue create \
--repo "$repo" \
--title "Further extraction needed: bump.py and publish.py still exceed 400-line guideline" \
--assignee "leynos" \
--body "$body"Length of output: 3908 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="leynos/lading"
body=$(cat <<'EOF'
## Summary
Follow-up from PR `#136` (issue `#108`): two source files still exceed the project's 400-line guideline and require further extraction beyond what was completed in that PR.
## Affected files
- `lading/commands/bump.py` — still ~27 lines over the 400-line limit. Docstring trimming alone cannot close the gap; further logic extraction is required (in the spirit of the `bump_manifests.py` split already landed).
- `lading/commands/publish.py` — 741 lines, deliberately deferred from PR `#136`.
## Rationale for deferral
Extracting `publish.py` is a substantial, non-minimal task because it requires:
1. Reconciling main's `#73/`#96 refactor with the pipeline extraction done in PR `#136` (`lading/commands/publish_pipeline.py`).
2. Rewriting the many `monkeypatch.setattr(publish, …)` test seams to target `publish_pipeline` instead, per the project's no-compat convention (no shim/back-compat layers).
This was judged unsuited to the review-fix pass on PR `#136` and tracked here as the agreed follow-up.
## Acceptance criteria
- [ ] `lading/commands/bump.py` is at or below 400 lines through further logic extraction (not just docstring trimming).
- [ ] `lading/commands/publish.py` is at or below 400 lines, with responsibilities distributed to `publish_pipeline.py` and/or other new modules as appropriate.
- [ ] All `monkeypatch.setattr(publish, …)` test seams are updated to target the correct module post-extraction, with no compatibility shims left behind.
- [ ] `docs/developers-guide.md` extraction map is updated to reflect the new module boundaries.
- [ ] `make lint`, `make typecheck`, and `make test` pass.
## References
- PR: https://github.com/leynos/lading/pull/136
- Requested by: `@leynos`
EOF
)
gh issue create \
--repo "$repo" \
--title "Further extraction needed: bump.py and publish.py still exceed 400-line guideline" \
--assignee "leynos" \
--body "$body"Length of output: 2151
It covers both remaining oversized files, (•‿•) |
This comment was marked as resolved.
This comment was marked as resolved.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "--- bump_manifests.py _BumpContext docstring ---"
rg -n "_BumpContext" -A 20 lading/commands/bump_manifests.py | head -40
echo "--- _apply_crate_manifest_update / _update_crate_manifest usage ---"
rg -n "_apply_crate_manifest_update|_update_crate_manifest" lading/commands/*.py
echo "--- non_negative_int ---"
rg -n "" lading/toml_coerce/_scalars.py | sed -n '1,140p'
echo "--- expect_sequence ---"
rg -n "" lading/toml_coerce/_sequences.py | sed -n '1,145p'
echo "--- graph_build.py docstring ---"
sed -n '1,15p' lading/workspace/graph_build.py
echo "--- string_matrix / _validate_matrix_entry ---"
rg -n "_reject|error\(" lading/toml_coerce/_sequences.pyLength of output: 12122 🏁 Script executed: #!/bin/bash
echo "--- snapshot file ---"
cat tests/unit/__snapshots__/test_toml_coerce.ambr
echo "--- test using preflight.aux_build ---"
rg -n "aux_build|preflight" tests/unit/test_toml_coerce.py -A3 -B3Length of output: 1847
No spelling-convention (-ise/-yse/-our) or complexity-increasing requirements survive in the applied changes; everything above already lands on the en-GB-oxendict side and none of it adds branching. Nothing further needed from this batch. (=^･ω･^=) all five plus the outside-diff item check out |
Resolve the CodeScene "Code Duplication" finding across the four rejection tests. Extract the common raise/capture/assert-substring/assert-type-name tail into a generic `_assert_rejection(call, expected_prefix, bad_value, error)` helper. `_assert_rejects_indexed_non_string` now builds the indexed input and delegates to it, and the two `string_matrix` rejection tests call it directly instead of repeating the tail inline. The `string_tuple` and `validate_string_sequence` rejection tests are unchanged — they already route through `_assert_rejects_indexed_non_string`. All four tests stay distinct, independently discoverable methods; assertion semantics and field-name literals are unchanged. (The helper's summary docstring is wrapped to keep it within the line-length limit.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary
Closes #108
lading/toml_coerce.py— new canonical home for the TOML scalar/sequence/mapping coercion helpers previously duplicated byconfig.pyandworkspace/models.py. Helpers take anerrorkeyword naming theLadingErrorsubclass to raise; both consumers bind their domain error type once (functools.partial, or a typed wrapper where overload narrowing must survive). One canonical message shape:{field} must be {expected}; received {type}.workspace/graph_build.py— cargo-metadata→model builders out ofmodels.py(which keeps the structures and topology).commands/publish_pipeline.py— per-crate package/publish pipeline, result classification, and mode dispatch out ofpublish.py(re-split after merging main's cargo-output-adapter rework from Move cargo output parsing to an adapter (#69) #88).commands/bump_manifests.py— per-manifest rewriting out ofbump.py.cli_options.py— Cyclopts argument declarations and version-argument validation out ofcli.py;bump_docs.pycondenses a duplicative module docstring.Every extraction is re-exported from its original home so existing import paths and test access keep resolving. All source files are now ≤398 lines. The developers' guide documents the coercion module, its error convention, and the extraction map.
Testing
tests/unit/test_toml_coerce.py: Hypothesis property tests for each coercion helper (well-typed pass-through; ill-typed raise with the canonical message includingtype(x).__name__), plus a syrupy snapshot of representative messages.make check-fmt,make lint,make typecheck, andmake test(594 passed) all green after merging currentmain.coderabbit review --agent: 0 findings.References
🤖 Generated with Claude Code