Fix mixed Google and NumPy docstrings (#162) - #199
Conversation
|
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:
Summary
Validation
WalkthroughThe pull request standardises NumPy-style documentation, enables documentation linting, updates selected type annotations, clarifies lockfile errors, and consolidates selected BDD, E2E, and unit-test helpers. ChangesDocumentation alignment
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 warnings, 2 inconclusive)
✅ Passed checks (15 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 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lading/commands/bump_toml.py`:
- Around line 133-148: Add concise, runnable Examples sections to the public
docstrings for update_section in lading/commands/bump_toml.py (lines 133-148),
update_dependency_sections in lading/commands/bump_toml.py (lines 165-180), and
make_preflight_config in tests/unit/publish/conftest.py (lines 80-89). Each
example must show representative inputs and the resulting bool or configuration,
including a dependency override for make_preflight_config.
🪄 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: 15432a03-d19a-4cf1-92a0-4ad44de9b68f
📒 Files selected for processing (4)
lading/commands/bump_manifests.pylading/commands/bump_toml.pylading/config.pytests/unit/publish/conftest.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/cuprum(auto-detected)leynos/shared-actions(auto-detected)
46e0660 to
bbf56ae
Compare
bb7d284 to
01a3bca
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
@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. Code Duplicationlading/commands/bump_lockfiles.py: What lead to degradation?The module contains 2 functions with similar structure: CargoLockfileRepository.regenerate_lockfiles,CargoLockfileRepository.resolve_lockfile_paths Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
@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. Code Duplicationtests/bdd/steps/test_bump_steps.py: What lead to degradation?The module contains 2 functions with similar structure: when_invoke_lading_bump,when_invoke_lading_bump_dry_run Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
|
@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. Code Duplicationtests/bdd/steps/test_publish_helpers.py: What lead to degradation?The module contains 4 functions with similar structure: _get_package_invocations,_get_publish_invocations,_get_test_invocation_envs,_get_test_invocations Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
|
@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. Code Duplicationtests/e2e/steps/test_e2e_steps.py: What lead to degradation?The module contains 3 functions with similar structure: when_run_lading_bump,when_run_lading_publish,when_run_lading_publish_allow_dirty Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
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
set -euo pipefail
printf '%s\n' '--- target module outline ---'
ast-grep outline lading/workspace/metadata.py --items all --view expanded
printf '%s\n' '--- error factories and parser context ---'
rg -n -A150 -B18 \
'^class CargoMetadataParseError\b|^\s+def (invalid_json|non_object_payload)\b|^def (_parse_cargo_metadata|load_cargo_metadata)\b' \
lading/workspace/metadata.py
printf '%s\n' '--- references and assertions ---'
rg -n -C4 \
'\bCargoMetadataParseError\.(invalid_json|non_object_payload)\s*\(|cargo metadata produced invalid JSON output|cargo metadata produced a non-object JSON payload' \
lading tests -g '*.py' || true
printf '%s\n' '--- focused test files ---'
fd -t f -e py . tests | rg 'metadata|workspace' | sort
printf '%s\n' '--- current target-file diff ---'
git diff --no-ext-diff --unified=12 -- lading/workspace/metadata.pyLength of output: 980
Do not apply a fix or suppress the CodeScene diagnostic yet. Current-codegraph verification failed because repository setup failed before the inspection commands ran. I could not verify the current implementations, parser call sites, tests, or PR diff. Retry the request after repository access is available. Keep this finding unresolved until that inspection completes. You are interacting with an AI system. |
_raise_aggregated_failure branches on len(manifests) == 1, so describing the re-raise path in terms of how many lockfiles were regenerated is too narrow -- and doubly wrong, since a failed manifest produces no regenerated lockfile at all. Every manifest is attempted, so the distinction is the attempt count. The regenerate_lockfiles docstring was corrected in 5af1852; this applies the same correction to the three surviving prose copies in the user and developer guides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9fe366b widened SubprocessContext.env and its consumers to Mapping[str, object], but left subprocess_runner's own env parameter as Mapping[str, str]. That made the public entry point the narrowest link in the chain: a caller could not pass a Path or int through subprocess_runner even though the context field, normalise_environment and the redacting logger all accept one. Widening a parameter is contravariance-safe, so subprocess_runner still satisfies the CommandRunner protocol; ty confirms this. The protocol itself is left alone -- narrowing that port would cascade to every implementation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai Have the following now been resolved? lading/testing/cmd_mox_runner.py (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Sources: Coding guidelines, Path instructions |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- declaration outlines ---'
ast-grep outline lading/testing/cmd_mox_runner.py --items all --view expanded
ast-grep outline tests/bdd/steps/test_publish_infrastructure.py --items all --view expanded
ast-grep outline tests/bdd/steps/test_publish_when_steps.py --items all --view expanded
printf '%s\n' '--- target declarations and context ---'
rg -n -A55 -B10 \
'^def (_resolve_cmd_mox_timeout|_process_cmd_mox_response|_should_namespace_cargo_command|_build_cmd_mox_passthrough_env|_merge_cmd_mox_path_entries|_cmd_mox_shim_directory|_normalise_preflight_responses|_invoke_publish_with_options)\b' \
lading/testing/cmd_mox_runner.py \
tests/bdd/steps/test_publish_infrastructure.py \
tests/bdd/steps/test_publish_when_steps.py
printf '%s\n' '--- target docstring shape summary ---'
python3 - <<'PY'
import ast
from pathlib import Path
targets = {
Path("lading/testing/cmd_mox_runner.py"): {
"_resolve_cmd_mox_timeout",
"_process_cmd_mox_response",
"_should_namespace_cargo_command",
"_build_cmd_mox_passthrough_env",
"_merge_cmd_mox_path_entries",
"_cmd_mox_shim_directory",
},
Path("tests/bdd/steps/test_publish_infrastructure.py"): {
"_normalise_preflight_responses",
},
Path("tests/bdd/steps/test_publish_when_steps.py"): {
"_invoke_publish_with_options",
},
}
for path, names in targets.items():
print(path)
tree = ast.parse(path.read_text())
found = set()
for node in ast.walk(tree):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name in names:
found.add(node.name)
doc = ast.get_docstring(node, clean=False)
lines = [] if doc is None else doc.splitlines()
print(f" {node.name}: docstring_lines={len(lines)}; first_line={lines[0] if lines else None!r}")
missing = names - found
if missing:
raise SystemExit(f"{path}: missing declarations: {sorted(missing)}")
PY
printf '%s\n' '--- current diff for target files ---'
git diff --no-ext-diff --unified=8 -- \
lading/testing/cmd_mox_runner.py \
tests/bdd/steps/test_publish_infrastructure.py \
tests/bdd/steps/test_publish_when_steps.pyLength of output: 34882
Treat this as static current-source verification. Do not claim runtime validation. You are interacting with an AI system. |
|
@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. Code Duplicationlading/workspace/metadata.py: What lead to degradation?The module contains 2 functions with similar structure: CargoMetadataParseError.invalid_json,CargoMetadataParseError.non_object_payload Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- target module outline ---'
ast-grep outline lading/workspace/metadata.py --items all --view expanded
printf '%s\n' '--- error factories and parsing context ---'
rg -n -A130 -B16 \
'^class CargoMetadataParseError\b|^\s+def (invalid_json|non_object_payload)\b|^def (_parse_cargo_metadata|load_cargo_metadata)\b' \
lading/workspace/metadata.py
printf '%s\n' '--- direct factory call sites and message assertions ---'
rg -n -C4 \
'\bCargoMetadataParseError\.(invalid_json|non_object_payload)\s*\(|cargo metadata produced invalid JSON output|cargo metadata produced a non-object JSON payload' \
lading tests -g '*.py' || true
printf '%s\n' '--- focused metadata test candidates ---'
fd -t f -e py . tests | rg 'metadata|workspace' | sort
printf '%s\n' '--- current PR diff for the target file ---'
git diff --no-ext-diff --unified=12 -- lading/workspace/metadata.pyLength of output: 1029 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- target declarations and parser context ---'
rg -n -A120 -B14 \
'^class CargoMetadataParseError\b|^\s+def (invalid_json|non_object_payload)\b|^def (_parse_cargo_metadata|load_cargo_metadata)\b' \
lading/workspace/metadata.py
printf '%s\n' '--- references and assertions ---'
rg -n -C4 \
'\bCargoMetadataParseError\.(invalid_json|non_object_payload)\s*\(|cargo metadata produced invalid JSON output|cargo metadata produced a non-object JSON payload' \
lading tests -g '*.py' || true
printf '%s\n' '--- target-file diff ---'
git diff --no-ext-diff --unified=12 -- lading/workspace/metadata.pyLength of output: 756
Keep this finding unresolved. The second current-codegraph inspection also failed during repository setup. No inspection command ran. I cannot verify the current implementations, their distinct parser branches, call sites, tests, or PR diff. Retry after repository access is available. Do not apply a refactor or suppress the CodeScene diagnostic until that verification completes. You are interacting with an AI system. |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
_expect_sequence: the discarded paragraph explained why the helper is a typed wrapper rather than functools.partial. That is design rationale rather than caller-facing behaviour, and the return-narrowing it describes is already documented on the two overload stubs, which are the caller-visible contract. The overloads and their docstrings are untouched. _coerce_publish_setting: its prose documented a coercion rule that is not recoverable from the (value: object, package_id: str) -> bool signature -- None and non-empty registry lists mean publishable, false and an empty list do not. AGENTS.md requires non-obvious behaviour to survive, so the replacement one-liner carries the rule itself rather than restating the function name. Both are now exactly one physical line, so ignore-one-line-docstrings exempts them from DOC201/DOC501 despite dropping Returns and Raises. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 276-278: Update the flowchart decision in the documented
error-handling flow to test whether one manifest was attempted, replacing the
workspace-root-specific question. Relabel the branches to distinguish “Yes, one
manifest attempted” from “No, multiple manifests attempted,” ensuring a single
nested-manifest failure follows the original Cargo error path and multiple
attempts follow the aggregate path.
In `@lading/runtime/subprocess_runner.py`:
- Line 51: Update the public SubprocessContext docstring to include a
NumPy-style env parameter section, documenting that env accepts non-string
values and that normalise_environment converts each value with str() before
subprocess execution.
🪄 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: 2f2409a1-aae0-42ac-a052-7719e198e567
📒 Files selected for processing (6)
docs/developers-guide.mddocs/users-guide.mdlading/config.pylading/runtime/subprocess_runner.pytests/bdd/steps/test_publish_infrastructure.pytests/bdd/steps/test_publish_when_steps.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/cuprum(auto-detected)leynos/shared-actions(auto-detected)
Introduce CliRunResult, a TypedDict with the four keys _run_cli actually produces, and use it wherever a publish step passes that dict around. The contract lives in its own module rather than in test_common_steps, which owns _run_cli. test_common_steps imports the bump and publish step modules at the bottom of the file so their steps register with pytest-bdd, which makes it a hub: test_common_steps -> test_publish_when_steps -> test_publish_infrastructure -> test_common_steps would close a real cycle. That is why _run_cli is reached through TYPE_CHECKING guards and deferred function-body imports today. cli_run_types imports no sibling step module, so it cannot join that cycle. pytest collects only test_*.py, so the module is not mistaken for a test, and the tests/bdd/steps/*.py per-file ignores still cover it. test_bump_steps already declared _BumpCliRun with these exact four fields, so the shared contract replaces it rather than sitting beside it. Typing _run_cli also makes the typ.cast in _invoke_lading_bump redundant, so it becomes a direct return. Every import of the contract sits in a TYPE_CHECKING block: the annotations are deferred by __future__.annotations and the type has no runtime use, so ruff's TC001 requires it there. The e2e run_cli helper is left alone -- it returns workspace_root plus an extra command key, so it does not fit this contract. The shared then_* steps in test_common_steps and test_bump_steps keep dict[str, typ.Any]; they are not publish-specific, and pytest-bdd injects them as fixtures, so no call edge forces the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
run_cli builds one dict literal with five unconditional keys, so declare _CliRunResult alongside it and return that instead of dict[str, typ.Any]. The shape stays exactly as it was: command is a list[str] and workspace_root is the Path passed through unconverted. This is a separate contract from the BDD tree's CliRunResult rather than a shared one: the e2e result carries an extra command key and names the path workspace_root, so the two cannot be unified without changing one of the dictionaries. Retype the consumers that actually hold a CLI result -- the private _run_lading_in_e2e_workspace helper, the three when_run_lading_* steps that target the cli_run fixture, and the two then steps that read it -- plus the Returns sections that named the old type. The e2e_state and publish_spies mappings keep dict[str, typ.Any]; they are unrelated bags, and e2e_state in particular gains a regenerated_marker key at runtime that a closed TypedDict would reject. _CliRunResult is imported under TYPE_CHECKING because test_e2e_steps only names it in annotations; ruff's TC001 requires that placement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeScene flagged invalid_json and non_object_payload as duplicated structure.
The duplication was real but the shared part was scaffolding, not behaviour:
each body was one return cls("<literal>"), and each was called exactly once,
on adjacent lines of _parse_cargo_metadata. Extracting a shared abstraction
would only have produced cls(message) -- the constructor that already exists.
Inlining removes 30 lines and makes the module consistent with itself: every
other raise here already constructs the error directly, so the factories were
the outlier rather than the pattern. The class is not re-exported and nothing
outside the two call sites referenced either factory.
This also retires a file-wide lint suppression. The DOC501/DOC502 ignore for
metadata.py existed because pydoclint keys those rules on the syntactic
raise-callable and could not resolve a factory classmethod to its exception
type. With the raises direct, only load_cargo_metadata still needs DOC502 --
for exceptions it genuinely propagates -- so a justified per-function noqa
replaces the file-wide entry, and the pyproject comment no longer describes
factories that have been deleted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Our agent can fix these. Install it.
Gates Passed
6 Quality Gates Passed
Absence of Expected Change Pattern
- lading/lading/commands/publish.py is usually changed with: lading/tests/bdd/steps/test_publish_steps.py
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Summary
This branch standardizes the affected docstrings on the repository's NumPy
conventions so that private helpers retain concise summaries and public
interfaces provide structured parameter and return documentation.
It also removes the remaining mixed Google/NumPy docstring found by the
whole-tree audit while preserving the deliberately pure-Google helper outwith
this issue's scope.
Closes #162.
Review walkthrough
Validation
make check-fmt: passed after every milestonemake typecheck: passed after every milestonemake lint: passed after every milestonemake test: 739 passed after every milestoneArgs:audit: no mixed Google/NumPy docstrings remaingit diff --check origin/main...HEAD: passedcoderabbit review --agent: zero findings after each of three milestones