Extract shared command-failure detail helpers (#102) - #115
Merged
Conversation
The "render command-failure detail" idiom — pick stderr, fall back to stdout, strip, and conditionally append to a message — was duplicated across lockfile.py (3 sites), publish_preflight.py (3 sites), publish_index_check.py, bump_lockfiles.py, and metadata.py. Add command_detail() and with_detail() to lading.utils.process as the canonical home and convert every site. The helpers strip before choosing, matching metadata.py's existing semantics: a whitespace-only stderr now falls back to stdout instead of yielding an empty detail. with_detail() accepts a separator so the auxiliary-build site keeps its "; " join. Add Hypothesis property tests covering the helper domain and syrupy snapshots pinning the rendered failure messages at each consuming boundary. Document the canonical helpers in the developers' guide. Closes #102
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
SummaryCentralises the Key Changes
Testing
Semantics and Behaviour Guarantees
Files Touched (high level)
WalkthroughExtract helpers command_detail, append_detail and with_detail into lading.utils.process, export them, adopt them across command and workspace modules, add Hypothesis property tests and snapshot tests, and document the canonical usage in the developer guide. ChangesCommand-failure detail helpers refactoring
Possibly related PRs
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideCentralizes the logic for deriving and appending command-failure detail from stdout/stderr into reusable helpers in lading.utils.process, refactors all call sites to use them (with consistent semantics and minor behavior tweaks), documents the helpers, and adds property and snapshot tests to pin their behavior and operator-facing messages. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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.
leynos
marked this pull request as ready for review
June 10, 2026 11:19
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Resolve the code-review comments on the issue #102 extraction: - Add `append_detail(message, detail, *, separator=": ")` to `lading.utils.process` for appending an already-derived detail, and reimplement `with_detail` as the derive-then-append wrapper over it. `_verify_clean_working_tree` now derives the detail once (it must inspect it to choose the base message) instead of recomputing via `with_detail`. - Refactor `_handle_git_ls_files_failure` to render its message through `append_detail` rather than the bespoke `detail or fallback` f-string, with a comment explaining why this site keeps a status-code fallback. - Expand the `process` module docstring to document its purpose, the three failure-detail helpers, and the dependent modules that must use them. - Restructure the cross-cutting failure-message snapshots to drive only public command entry points (`refresh_lockfile`, `regenerate_lockfiles`, `load_cargo_metadata`) with an injected failing runner, so they exercise real failure paths rather than calling private formatters with mocks. The pre-flight message snapshot moves to its own boundary (`_run_cargo_preflight`) in test_preflight_cargo_runner.py; the package/publish message is already snapshotted in test_packaging.py. - Add property and unit coverage for `append_detail`. All snapshot text is byte-identical to before, so operator-facing messages are unchanged. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/developers-guide.md`:
- Around line 454-456: Update the module list to use fully qualified names to
match the rest of the document: replace bare names `lockfile`, `bump_lockfiles`,
`publish_preflight`, `publish_index_check`, and `workspace.metadata` with their
package-qualified counterparts (e.g., `lading.commands.lockfile`,
`lading.commands.bump_lockfiles`, `lading.commands.publish_preflight`,
`lading.commands.publish_index_check`, `lading.workspace.metadata` or the
appropriate `lading.*` paths used elsewhere) so the naming convention matches
examples like `lading.commands.bump` and `lading.commands.bump_output`.
In `@lading/utils/process.py`:
- Around line 73-129: The docstrings for the public helper functions
command_detail, append_detail, and with_detail do not follow the repository's
NumPy-style docstring contract; update each docstring to include explicit
"Parameters" and "Returns" sections (and brief one-line summary) using NumPy
structured format: list each parameter name, type, and short description (e.g.,
message: str — description; stdout: str — description; stderr: str —
description; separator: str, optional — description), and describe the return
value and its type under a "Returns" section (e.g., str — description). Keep
examples if present under an "Examples" section and ensure visibility that these
are public helpers.
In `@tests/unit/publish/test_preflight_cargo_runner.py`:
- Around line 55-61: The test function
test_run_cargo_preflight_failure_message_snapshot currently takes multiple
scalar parameters (tmp_path, snapshot, subcommand, exit_code, stderr); refactor
it to accept a single case object (e.g., a dataclass or dict named something
like case) that bundles subcommand, exit_code, and stderr so the argument count
is reduced; update the parametrize decorator to yield that case object and
adjust references inside the test from subcommand/exit_code/stderr to
case.subcommand/case.exit_code/case.stderr while keeping tmp_path and snapshot
as fixtures.
🪄 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: 3e5e5d8a-9ea8-4410-ad50-6b7bc6d5f1df
📒 Files selected for processing (12)
docs/developers-guide.mdlading/commands/bump_lockfiles.pylading/commands/lockfile.pylading/commands/publish_index_check.pylading/commands/publish_preflight.pylading/utils/process.pylading/workspace/metadata.pytests/unit/__snapshots__/test_command_failure_messages.ambrtests/unit/publish/__snapshots__/test_preflight_cargo_runner.ambrtests/unit/publish/test_preflight_cargo_runner.pytests/unit/test_command_failure_messages.pytests/unit/utils/test_process.py
Owner
Author
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check current state of publish_preflight.py for _verify_clean_working_tree
rg -n "command_detail|with_detail|append_detail|_verify_clean_working_tree" lading/commands/publish_preflight.py -A3 -B3
echo "---"
# Check _handle_git_ls_files_failure in lockfile.py
rg -n "command_detail|with_detail|append_detail|_handle_git_ls_files_failure|detail or" lading/commands/lockfile.py -A5 -B3Length of output: 5714
Both concerns are fully addressed. |
- Fully-qualify the dependent module names in the developer guide's command-failure detail helper section (`lading.commands.lockfile`, etc.) to match the document's existing naming convention. - Add NumPy-style Parameters/Returns sections to the public `command_detail`, `append_detail`, and `with_detail` helpers, per the repository docstring contract in .rules/python-00.md. - Bundle the cargo pre-flight failure-message snapshot test's scalar parameters into a `_PreflightFailureCase` dataclass, matching the case-object pattern already used in that file and reducing the test's argument count. Parametrize ids are unchanged, so snapshots are stable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lodyai Bot
pushed a commit
that referenced
this pull request
Jul 7, 2026
main's PR #102/#115 added test_lockfile_regeneration_failure_message, which snapshots regenerate_lockfiles' failure string. That snapshot pinned the pre-#84 single-error format; this branch (issue #84) now raises one aggregated LockfileRegenerationError that lists each failed manifest with its repair command. Refresh the snapshot to the aggregated message. The test's intent — verifying the canonical `with_detail` suffix is rendered — is preserved: the per-manifest line still carries "...with exit code 101: error: dependency conflict". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #102
command_detail(stdout, stderr)andwith_detail(message, stdout, stderr, *, separator=": ")tolading/utils/process.py— the canonical home for the command-failure detail idiom.lockfile.py(3×),publish_preflight.py(3×),publish_index_check.py,bump_lockfiles.py, andworkspace/metadata.py. No inline(stderr or stdout).strip()remains.metadata.py's previous behaviour) — a whitespace-only stderr now falls back to stdout rather than yielding an empty detail.with_detailaccepts a separator so the auxiliary-build site keeps its"; "join.docs/developers-guide.md.Testing
(stdout, stderr)pairs: stderr wins when non-empty after strip, stdout is the fallback, no suffix when both strip empty, output always stripped.tests/unit/test_command_failure_messages.pysnapshots the rendered failure messages at each consuming boundary (cargo preflight, package/publish, lockfile refresh/regeneration, cargo metadata) so the extraction cannot silently change operator-facing text.make check-fmt,make lint,make typecheck, andmake test(563 passed) all green.coderabbit review --agent: 0 findings.🤖 Generated with Claude Code
Summary by Sourcery
Centralize command-failure detail formatting in shared helpers and update all call sites to use them while preserving operator-facing messages.
Enhancements:
Documentation:
Tests: