Fix mixed Google and NumPy docstrings (#162) - #199
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
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 passed for formatting, type checking, linting, 739 tests, whole-tree WalkthroughThe change standardises production and test docstrings around NumPy-style sections, enables Ruff’s ChangesDocumentation alignment
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (18 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![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.
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: 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)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
46e0660 to
bbf56ae
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.
bb7d284 to
01a3bca
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 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.
There was a problem hiding this comment.
Actionable comments posted: 33
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
lading/runtime/subprocess_runner.py (1)
80-81: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the exception contracts removed from public subprocess APIs.
Restore the
Raisessections:subprocess_runnerstill exposesValueErrorandCommandSpawnError,invoke_via_subprocessstill propagatesCommandSpawnErrorfrom_spawn_process, andwrite_to_sinkstill allows non-BrokenPipeErrorfailures fromsink.write()andsink.flush()to escape.As per coding guidelines, public interfaces must retain structured exception documentation where applicable.
🤖 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/runtime/subprocess_runner.py` around lines 80 - 81, Restore the structured Raises documentation in lading/runtime/subprocess_runner.py at lines 80-81, 195-196, and 319-320: document ValueError and CommandSpawnError for subprocess_runner, document propagation of CommandSpawnError from _spawn_process in invoke_via_subprocess, and document that write_to_sink allows non-BrokenPipeError failures from sink.write() and sink.flush() to escape.Source: Coding guidelines
tests/unit/publish/test_preflight_lockfile_validation.py (1)
75-86: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCollapse private test-helper docstrings to one-line summaries.
Replace the expanded
Returnssections on_repository_for_outcomesand_stale_lockfiles_error_messagewith concise single-line summaries.As per path instructions, private functions and methods require single-line summaries.
Also applies to: 107-117
🤖 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 `@tests/unit/publish/test_preflight_lockfile_validation.py` around lines 75 - 86, Condense the docstrings for the private helpers _repository_for_outcomes and _stale_lockfiles_error_message to single-line summaries, removing their expanded Returns sections while preserving the helpers’ purposes.Source: Path instructions
tests/unit/test_bump_lockfiles.py (1)
297-311: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCollapse private helper docstrings to one-line summaries.
Replace the expanded
Returnssections on these internal strategy and fixture helpers with concise summaries.As per path instructions, private functions and methods require single-line summaries.
Also applies to: 333-340, 445-451, 475-481
🤖 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 `@tests/unit/test_bump_lockfiles.py` around lines 297 - 311, The private helper _inside_dir has an expanded multi-line docstring with a Returns section. Replace it with a concise single-line summary, preserving the description that it generates relative directory paths contained within the workspace; apply the same collapse to the private helpers at the additional referenced locations.Source: Path instructions
lading/commands/bump_readme.py (1)
263-269: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the public
Raisescontract.Document that
transpose_readme_to_cratecan propagateReadmeTranspositionErrorfrom_load_source_textand_resolve_crate_relative_path. Removing this section leaves the public API documentation incomplete.As per path instructions, public interfaces require structured documentation for applicable raised exceptions.
🤖 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_readme.py` around lines 263 - 269, Update the docstring for transpose_readme_to_crate to restore a structured Raises section documenting ReadmeTranspositionError propagated from _load_source_text and _resolve_crate_relative_path. Keep the existing Returns documentation unchanged.Source: Path instructions
lading/toml_coerce/_mappings.py (1)
42-61: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
optional_mappingfailures.Add a
Raisessection explaining the exception produced byerrorwhen a non-Nonevalue is not a mapping. This function delegates toexpect_mapping, so invalid input is not an unhandled no-error path.🤖 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/_mappings.py` around lines 42 - 61, Update the optional_mapping docstring to add a Raises section documenting that the exception produced by the error factory is raised when a non-None value is not a mapping, consistent with its delegation to expect_mapping.
🤖 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/cli.py`:
- Around line 70-82: Reduce the docstrings for all listed private helpers to a
single-line summary, removing structured Returns and Raises sections while
preserving each helper’s essential description. Apply this to the named symbols
in lading/cli.py, lading/cli_options.py, lading/runtime/subprocess_runner.py,
tests/e2e/steps/test_e2e_steps.py, tests/unit/publish/test_snapshot_messages.py,
tests/unit/test_bump_rebuild_lockfiles_resolution.py, tests/unit/test_cli.py,
and tests/unit/test_subprocess_runner_logging.py at the specified ranges; public
interfaces remain unchanged with full NumPy-style documentation.
- Around line 322-328: Complete the NumPy-style docstrings for every listed
callable: add Parameters entries with accurate types and descriptions for all
named arguments across lading/cli.py, tests/conftest.py, tests/e2e/conftest.py,
tests/e2e/helpers/e2e_steps_helpers.py, and tests/e2e/steps/test_e2e_steps.py.
Update the Raises sections in the specified E2E helpers and step functions to
name E2EExpectationError, while keeping dependency_entry_not_string,
staging_root_missing, and unsupported_fixture_version documented as factory
methods rather than exception classes; ensure each public interface has a
complete structured contract.
In `@lading/commands/_shared.py`:
- Around line 12-18: Complete the public docstrings with structured parameter
descriptions and runnable examples showing outcomes: in
lading/commands/_shared.py:12-18 document workspace and demonstrate
singular/plural crate summaries; in lading/commands/bump.py:132-139 document all
three parameters and show a representative run call; and in
lading/commands/bump_toml.py:43-65 (value_as_string, compose_requirement), 79-86
(prepare_version_replacement), 104-110 (assign_dependency_version_field),
125-131 (update_dependency_entry), 146-152 (update_dependency_table), 245-252
(parse_manifest), 261-267 (select_table), 283-289 (assign_version), and 306-312
(value_matches), add complete parameter documentation plus runnable examples
demonstrating usage and resulting behavior.
In `@lading/commands/bump_lockfiles.py`:
- Around line 126-133: Update the docstring for the error-handling function
around the “Raise the appropriate error” documentation to describe behavior
based on the number of attempted manifests: one manifest re-raises its original
error, while multiple manifests raise an aggregated error listing each manifest.
Do not describe the condition in terms of the number of failures.
- Around line 33-40: Complete the NumPy-style documentation contract across all
listed sites: in lading/commands/bump_lockfiles.py ranges 33-40, document
workspace_root and lockfile_manifests; ranges 45-79, restore the
LockfileRegenerationError Raises contract and add a runnable example; ranges
295-311 and 319-335, add typed parameter descriptions and propagated failure
documentation. In lading/utils/metrics.py ranges 76-83 and 101-108, document
name and labels with typed Parameters and the requested zero-default or
empty-stats examples, and in 119-125 add a concise snapshot example. In
lading/utils/process.py ranges 51-58, document command and add an example. Add
typed parameter sections for workspace_root and manifest_path in
tests/unit/publish/test_preflight_lockfile_validation.py ranges 41-47 and 54-60,
and for command/cwd and tmp_path in tests/unit/test_bump_lockfiles.py ranges
49-55 and 512-518; preserve the existing APIs and behavior while ensuring every
public function, protocol method, fixture, and callable interface has complete
documentation.
- Around line 102-108: Collapse the structured docstrings on the
underscore-prefixed helper functions, including the helper documenting
regenerated lockfile paths and failures and the additional noted ranges, into
concise single-line summaries. Remove their Returns and Raises sections, while
retaining detailed documentation only for public interfaces.
In `@lading/commands/bump_readme.py`:
- Around line 108-115: The private-helper docstrings use unnecessary structured
sections instead of the required single-line summaries. In
lading/commands/bump_readme.py, simplify docstrings for
_rewrite_relative_link_match, _rewrite_links_outside_code, _load_source_text,
_resolve_crate_relative_path, _write_or_skip_readme,
_should_rewrite_link_target, and _has_uri_scheme; in
lading/commands/publish_manifest.py, simplify _load_manifest_document,
_write_manifest_document, _remove_per_crate_entries, _resolve_patch_tables,
_validate_and_load_manifest, and _apply_strategy_to_patches; in
lading/config.py, simplify _validate_mapping_keys and _strip_patches; in
lading/toml_coerce/_mappings.py, simplify _validate_string_pair; in
tests/bdd/steps/manifest_fixtures.py, simplify _update_manifest_version and
_update_crate_manifests; in tests/bdd/steps/metadata_fixtures.py, simplify
_write_workspace_readme; and in tests/unit/publish/test_run_workspace_config.py,
simplify fake_load, raise_not_loaded, capture_load, _normalise_summary,
raise_missing, raise_not_loaded, and raise_config_error. Remove Returns,
Parameters, and other structured sections while retaining concise descriptions
of each helper’s purpose.
In `@lading/commands/bump.py`:
- Around line 298-309: Update the Returns description in the README
transposition function’s docstring to say the returned crate README paths were
written or reported as would be written, accurately covering dry-run results
while preserving the existing return type and behavior.
- Around line 171-178: Reduce the docstrings for private underscore-prefixed
helpers to one concise summary, removing Returns and Raises sections while
preserving their meaning; retain structured documentation only for public
interfaces. Apply this at lading/commands/bump.py lines 171-178, 279-285,
298-309, 347-354, 372-379, and 417-421; lading/commands/bump_docs.py lines
151-157, 304-310, and 322-328; lading/commands/bump_manifests.py lines 64-70,
99-106, 121-126, 147-154, and 165-172; lading/commands/bump_output.py lines
81-85, 104-110, 127-137, 139-146, 162-168, and 199-205;
tests/helpers/workspace_builders.py lines 30-36, 72-83, 118-124, 133-144,
162-168, 179-185, and 208-214; tests/unit/test_bump_context_properties.py lines
68-73, 167-173, 183-189, 232-238, and 265-270;
tests/unit/test_bump_manifest_updates.py lines 44-50;
tests/unit/test_bump_result_formatting.py lines 39-45, 83-89, and 121-127; and
tests/unit/test_dependency_sections.py lines 41-47.
In `@lading/commands/cargo_output_adapter.py`:
- Around line 172-178: Shorten private and local helper docstrings to
single-line summaries, removing all structured Returns sections while preserving
concise descriptions: lading/commands/cargo_output_adapter.py lines 172-178,
191-198, and 222-229; lading/commands/publish.py lines 186-197, 222-231,
285-291, 302-314, 402-409, 458-462, 525-532, 579-588, 631-637, 650-661, and
675-681; lading/commands/publish_execution.py lines 16-22 and 33-45;
lading/commands/publish_index_check.py lines 60-65, 95-102, 241-245, and
278-283; tests/bdd/steps/test_publish_helpers.py lines 26-30, 44-66, 80-100,
109-133, 143-154, 164-175, 185-191, 202-227, 237-243, 258-264, 276-282, and
318-324; tests/unit/publish/preflight_test_utils.py lines 29-35, 51-57, and
69-75; tests/unit/publish/test_index_order_properties.py lines 28-34;
tests/unit/publish/test_publish_execution_helpers.py lines 49-55, 69-75, and
81-87; tests/unit/publish/test_run_preflight.py lines 76-82, 269-280, 312-318,
and 365-371; tests/unit/test_publish_planning.py lines 28-33, 47-52, and 76-81.
No direct changes are needed outside these documented private or local helpers.
In `@lading/commands/lockfile.py`:
- Around line 82-94: Collapse the expanded docstrings for the private helpers in
this module, including the nested runner and the helpers at the referenced
locations, into concise single-line summaries. Preserve the existing behavior
and retain only each helper’s brief purpose; remove the Returns and Raises
sections from these private-function docstrings.
- Around line 82-94: Update the docstring for the lockfile discovery function to
state that an empty tuple is returned only when the workspace is not a Git
repository, while non-zero Git exits for other reasons raise
LockfileDiscoveryError. Keep the documented successful return value and
exception behavior consistent with the implementation.
In `@lading/commands/publish_diagnostics.py`:
- Around line 12-29: Apply the private-helper documentation rule without
changing behavior: replace each structured docstring with a single-line summary
for _trim_artifact_token and _discover_stderr_artifacts in
lading/commands/publish_diagnostics.py:12-29, _read_tail_lines at 42-49, and
_format_artifact_diagnostics at 61-67, _append_compiletest_diagnostics at 88-95;
for _build_preflight_environment at lading/commands/publish_preflight.py:65-71,
_run_aux_build_commands at 84-90, _resolve_extern_path at 107-114,
_apply_compiletest_externs at 127-134, _compose_preflight_arguments at 222-228,
_collect_stale_lockfiles at 249-253, _build_stale_lockfile_message at 276-283,
_validate_lockfile_freshness at 312-316, _preflight_argument_sets at 333-339,
_normalise_test_excludes and _build_test_arguments at 348-367,
_verify_clean_working_tree at 384-390, _run_cargo_preflight at 428-434, and
_build_cargo_error_message at 459-465; make the same documentation-only change
for _raise_unquoted_args_error at
tests/bdd/steps/test_commands_catalogue_steps.py:46-52, _parse_quoted_args at
81-85, _construct_command_with_args at 120-126, _extract_dependency_requirement
at tests/bdd/steps/test_common_steps.py:103-115,
_then_dependency_requirement_step at 173-187, _create_test_crate at
tests/bdd/steps/test_data_helpers.py:18-24, _build_package_metadata at 51-57,
_validate_stub_arguments at
tests/bdd/steps/test_publish_infrastructure.py:103-110,
_resolve_preflight_expectation at 131-137, _is_cargo_publish_command at 150-156,
_make_preflight_handler at 166-172, _matches_expected_prefix at 193-199,
_make_preflight_dispatch_handler at 212-218, _existing_static_stub_response at
249-256, _create_stub_config at 281-287, _normalise_preflight_responses at
301-305, _make_git_handler at 413-419, _build_env_restore_dict at 442-448,
_invoke_publish_with_options at 474-480, _has_ordered_args_single at
tests/bdd/steps/test_publish_then_steps.py:122-129,
_run_and_record_cargo_preflight at
tests/unit/publish/test_preflight_cargo_runner.py:128-133, and _load, _triggers,
and _mutation_job at tests/workflow_contracts/test_mutation_testing.py:56-86.
In `@lading/commands/publish_execution.py`:
- Around line 40-44: Update the Raises documentation for the publish execution
command to name PublishPreflightError as the concrete raised exception,
replacing the _publish_error factory reference while preserving the existing
description.
In `@lading/commands/publish_plan.py`:
- Around line 43-50: Collapse the expanded private-helper docstrings into
concise single-line summaries for _categorize_crates,
_process_order_and_collect_errors, _build_order_validation_messages,
_resolve_configured_order, and _resolve_topological_order in
lading/commands/publish_plan.py; _is_ordering_dependency,
_build_dependency_graph, _initialize_topological_structures, _perform_kahn_sort,
and _collect_cycle_nodes in lading/workspace/models.py; _create_crate and
_build_cargo_metadata_payload in tests/e2e/helpers/workspace_builder.py;
_warning_records and nested _runner in tests/unit/publish/conftest.py;
_both_markers_stderr and _parse_index_lookup_failure in
tests/unit/publish/test_cargo_output_adapter.py;
_make_beta_package_index_failure_runner in
tests/unit/publish/test_downgrade_metrics.py; the outer helper and nested runner
in tests/unit/publish/test_run_publish_ordering.py; _apply_strategy_and_parse in
tests/unit/test_publish_patch_strategy.py; and _build_two_crate_metadata in
tests/unit/test_workspace_graph_building.py. Preserve each helper’s existing
meaning while removing expanded sections and keeping only a one-line summary.
- Around line 34-36: Complete NumPy-style public documentation with clear usage
examples and outcomes: in lading/commands/publish_plan.py lines 34-36 add
Returns for publishable_names, and lines 191-203 add Parameters and a runnable
Examples section for plan_publication; in lading/workspace/models.py lines
184-195 add an example for topologically_sorted_crates, and lines 213-216 add
structured Returns and an example for crates_by_name; document the specified
parameters and add runnable examples in tests/e2e/helpers/git_helpers.py lines
71-80 and 83-91, tests/e2e/helpers/workspace_builder.py lines 32-38,
tests/helpers/workspace_metadata.py lines 47-53 and 67-84,
tests/unit/publish/conftest.py lines 123-129, 145-151, 173-179, 189-196,
207-213, 264-270, 278-284, 313-319, 375-381, 398-404, and 421-427; add
structured Returns for calls at tests/unit/publish/conftest.py lines 365-366;
and document make_crate with an example in
tests/unit/test_publish_patch_strategy.py lines 37-43, preserving each
function’s existing behavior.
In `@lading/commands/publish_preflight.py`:
- Around line 312-316: Update the Raises documentation for
_validate_lockfile_freshness to include PublishPreflightError when
_collect_stale_lockfiles encounters an unexpected freshness-probe failure, in
addition to stale Cargo.lock files. Alternatively, remove the structured Raises
block if this private-helper documentation follows the project’s private-helper
rule.
In `@lading/commands/publish.py`:
- Around line 250-256: Complete the public docstrings for the staging and
publishing functions in lading/commands/publish.py (250-256 and 744-750) with
typed Parameters, applicable Raises, and runnable Examples showing usage and
outcome. Add NumPy-style Parameters documentation for every fixture argument in
tests/unit/conftest.py (171-177, 191-197, 215-221, 227-233, 260-266, 272-278,
and 286-292), and document tmp_path in
tests/unit/publish/test_publish_execution_helpers.py (94-101).
In `@lading/config.py`:
- Around line 74-81: Complete the NumPy-style documentation for the specified
public functions by adding typed Parameters sections for every listed input:
mapping in lading/config.py lines 74-81, 103-110, 140-147, 181-188, and 234-240;
workspace_root at 287-293 and 328-334; loader at 305-316; tmp_path at
tests/bdd/steps/config_fixtures.py lines 43-49 and 62-69; workspace_directory
and pattern at 73-80; workspace_directory and command at 191-197;
workspace_directory and count at 256-262; version, workspace_directory, and
repo_root in tests/bdd/steps/test_bump_steps.py lines 52-58 and 73-79; and all
five inputs at test_publish_given_steps.py lines 230-236 plus tmp_path, cmd_mox,
and monkeypatch at 253-259. Use accurate type annotations and retain the
existing Returns and Raises sections.
In `@lading/testing/cmd_mox_runner.py`:
- Around line 127-135: Update the Raises documentation for
_prepare_cmd_mox_context to state that CmdMoxError is raised when
CMOX_IPC_SOCKET is unset or CMOX_IPC_TIMEOUT is unparseable, non-finite, or
non-positive, matching _resolve_cmd_mox_timeout behavior.
In `@lading/toml_coerce/_mappings.py`:
- Around line 30-33: Replace the `_reject` entry in every Raises section with
documentation of the concrete exception type produced by the injected `error`
parameter, preserving the specific validation context for `expect_mapping`,
`_validate_string_pair`, `string_mapping`, `expect_string`, `boolean`, and
`non_negative_int`. Apply this in lading/toml_coerce/_mappings.py lines 30-33,
77-80, and 116-120, and lading/toml_coerce/_scalars.py lines 25-30, 59-64, and
95-101.
In `@lading/toml_coerce/_scalars.py`:
- Around line 97-100: Update the _reject exception documentation in _scalars.py
to state that rejection occurs when value is neither a real integer nor an
integer-valued string, or when the parsed result is negative. Preserve the
existing distinction that integer-valued strings are accepted, as shown by the
parsing logic around lines 112–116.
In `@lading/toml_coerce/_sequences.py`:
- Around line 50-54: Update the Raises sections for the five sequence helpers in
lading/toml_coerce/_sequences.py at lines 50-54, 95-99, 121-126, 156-160, and
187-192 to document the configured error exception type rather than the _reject
factory. In lading/workspace/metadata.py lines 173-179, document
CargoMetadataParseError instead of the invalid_json and non_object_payload
factories.
In `@lading/utils/metrics.py`:
- Around line 76-83: Update counter_value to retrieve the counter with a default
of 0 for missing keys, replacing direct dictionary indexing while preserving the
existing key construction via _counter_key.
- Around line 49-56: Collapse the docstring of the private _counter_key function
to a single-line summary describing that it returns a stable registry key from
the metric name and sorted labels; remove the structured Returns section.
In `@lading/workspace/models.py`:
- Around line 164-170: Update the docstring for _collect_cycle_nodes to describe
all nodes left unordered because dependency cycles block sorting, including
affected dependants that are not cycle members. Revise the summary and return
description without changing the method’s behavior.
In `@tests/bdd/steps/manifest_fixtures.py`:
- Around line 23-27: Update the Raises documentation for
_update_manifest_version to state that AssertionError occurs only when an
intermediate key in the nested path is missing. Do not claim failure for a
missing final key, since target[keys[-1]] = version creates it.
In `@tests/bdd/steps/test_common_steps.py`:
- Around line 143-149: Complete the NumPy-style documentation for every listed
public fixture or step: in tests/bdd/steps/test_common_steps.py:143-149 document
cli_run and check; in tests/bdd/steps/test_publish_fixtures.py:16-24 and 27-34
add concise runnable fixture examples, and at 44-50 document cmd_mox,
preflight_overrides, and preflight_recorder with an example; in
tests/bdd/steps/test_publish_infrastructure.py:78-84 document allow_dirty with
an example; in tests/bdd/steps/test_publish_then_steps.py:106-112, 136-142,
157-163, 176-182, 194-200, 212-218, and 297-303 add typed Parameters for the
specified arguments and concise runnable Examples; in
tests/bdd/steps/test_publish_when_steps.py:27-33, 45-56, 80-86, and 105-111
document all specified context/path or other arguments with typed Parameters and
runnable Examples. Include Examples sections for zero-argument fixtures as
requested.
In `@tests/unit/publish/test_cargo_output_adapter.py`:
- Around line 61-67: Update the private strategy function’s docstring summary to
one line describing that it independently selects uppercase or lowercase output
for each character in text, rather than toggling each character’s existing case;
remove the expanded Returns section.
In `@tests/unit/publish/test_publish_manifest_strategies.py`:
- Around line 19-25: Collapse the structured docstrings for the
underscore-prefixed test helpers in test_publish_manifest_strategies.py,
including the helper at the referenced additional location, into single-line
summaries. Remove the Returns sections while retaining each helper’s essential
purpose.
In `@tests/unit/test_bump_output_formatting.py`:
- Around line 168-174: Collapse the multi-line docstrings for the private
formatting helpers around the shown helper and the additional locations at
183-189 and 202-208 into single-line summaries. Remove their structured Returns
sections while preserving each helper’s purpose and return behavior.
In `@tests/unit/test_bump_readme.py`:
- Around line 38-44: Update the private fixture helper documented by the “Create
a workspace crate model rooted at relative_root” summary to use a concise
single-line docstring, removing the structured Returns section while preserving
the helper’s behavior.
In `@tests/unit/test_lockfile.py`:
- Around line 53-59: Condense the docstrings for the private and local test
helpers, including the stub runner at the diff location and the helpers at the
referenced locations, into single-line summaries. Remove their structured
Returns sections while preserving the existing description of each helper’s
behavior.
---
Outside diff comments:
In `@lading/commands/bump_readme.py`:
- Around line 263-269: Update the docstring for transpose_readme_to_crate to
restore a structured Raises section documenting ReadmeTranspositionError
propagated from _load_source_text and _resolve_crate_relative_path. Keep the
existing Returns documentation unchanged.
In `@lading/runtime/subprocess_runner.py`:
- Around line 80-81: Restore the structured Raises documentation in
lading/runtime/subprocess_runner.py at lines 80-81, 195-196, and 319-320:
document ValueError and CommandSpawnError for subprocess_runner, document
propagation of CommandSpawnError from _spawn_process in invoke_via_subprocess,
and document that write_to_sink allows non-BrokenPipeError failures from
sink.write() and sink.flush() to escape.
In `@lading/toml_coerce/_mappings.py`:
- Around line 42-61: Update the optional_mapping docstring to add a Raises
section documenting that the exception produced by the error factory is raised
when a non-None value is not a mapping, consistent with its delegation to
expect_mapping.
In `@tests/unit/publish/test_preflight_lockfile_validation.py`:
- Around line 75-86: Condense the docstrings for the private helpers
_repository_for_outcomes and _stale_lockfiles_error_message to single-line
summaries, removing their expanded Returns sections while preserving the
helpers’ purposes.
In `@tests/unit/test_bump_lockfiles.py`:
- Around line 297-311: The private helper _inside_dir has an expanded multi-line
docstring with a Returns section. Replace it with a concise single-line summary,
preserving the description that it generates relative directory paths contained
within the workspace; apply the same collapse to the private helpers at the
additional referenced locations.
🪄 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: 704e785f-5774-4db4-9f11-89b9e248ef47
📒 Files selected for processing (88)
lading/cli.pylading/cli_options.pylading/commands/_shared.pylading/commands/bump.pylading/commands/bump_docs.pylading/commands/bump_lockfiles.pylading/commands/bump_manifests.pylading/commands/bump_output.pylading/commands/bump_readme.pylading/commands/bump_toml.pylading/commands/cargo_output_adapter.pylading/commands/lockfile.pylading/commands/publish.pylading/commands/publish_diagnostics.pylading/commands/publish_execution.pylading/commands/publish_index_check.pylading/commands/publish_manifest.pylading/commands/publish_plan.pylading/commands/publish_preflight.pylading/config.pylading/runtime/runner.pylading/runtime/subprocess_runner.pylading/testing/cmd_mox_runner.pylading/testing/toml_utils.pylading/toml_coerce/_core.pylading/toml_coerce/_mappings.pylading/toml_coerce/_scalars.pylading/toml_coerce/_sequences.pylading/utils/metrics.pylading/utils/process.pylading/workspace/_coercion.pylading/workspace/graph_build.pylading/workspace/metadata.pylading/workspace/models.pypyproject.tomltests/bdd/steps/config_fixtures.pytests/bdd/steps/manifest_fixtures.pytests/bdd/steps/metadata_fixtures.pytests/bdd/steps/test_bump_steps.pytests/bdd/steps/test_commands_catalogue_steps.pytests/bdd/steps/test_common_steps.pytests/bdd/steps/test_data_helpers.pytests/bdd/steps/test_publish_fixtures.pytests/bdd/steps/test_publish_given_steps.pytests/bdd/steps/test_publish_helpers.pytests/bdd/steps/test_publish_infrastructure.pytests/bdd/steps/test_publish_then_steps.pytests/bdd/steps/test_publish_when_steps.pytests/bdd/steps/test_workspace_metadata_steps.pytests/conftest.pytests/e2e/conftest.pytests/e2e/helpers/e2e_steps_helpers.pytests/e2e/helpers/git_helpers.pytests/e2e/helpers/workspace_builder.pytests/e2e/steps/test_e2e_steps.pytests/helpers/workspace_builders.pytests/helpers/workspace_metadata.pytests/unit/conftest.pytests/unit/publish/conftest.pytests/unit/publish/preflight_test_utils.pytests/unit/publish/test_cargo_output_adapter.pytests/unit/publish/test_downgrade_metrics.pytests/unit/publish/test_index_order_properties.pytests/unit/publish/test_preflight_cargo_runner.pytests/unit/publish/test_preflight_lockfile_validation.pytests/unit/publish/test_publish_execution_helpers.pytests/unit/publish/test_publish_manifest_strategies.pytests/unit/publish/test_run_preflight.pytests/unit/publish/test_run_publish_ordering.pytests/unit/publish/test_run_workspace_config.pytests/unit/publish/test_snapshot_messages.pytests/unit/test_bump_context_properties.pytests/unit/test_bump_lockfiles.pytests/unit/test_bump_manifest_updates.pytests/unit/test_bump_output_formatting.pytests/unit/test_bump_readme.pytests/unit/test_bump_rebuild_lockfiles_resolution.pytests/unit/test_bump_result_formatting.pytests/unit/test_cli.pytests/unit/test_cmd_mox_integration.pytests/unit/test_command_failure_messages.pytests/unit/test_dependency_sections.pytests/unit/test_lockfile.pytests/unit/test_publish_patch_strategy.pytests/unit/test_publish_planning.pytests/unit/test_subprocess_runner_logging.pytests/unit/test_workspace_graph_building.pytests/workflow_contracts/test_mutation_testing.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)
💤 Files with no reviewable changes (1)
- lading/testing/toml_utils.py
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.
Keep private helper documentation to the required single-line summaries and remove structured sections intended only for public interfaces.
Document the public TOML update helpers with complete NumPy-style parameter and return sections.
Convert the remaining mixed Google and NumPy-style fixture docstring to the repository's structured public-interface format.
Show representative update results for the public TOML helpers and the preflight fixture's compiletest dependency override.
Add the pydoclint `DOC` rule group to the Ruff lint selection so that docstrings must document returns, yields, and directly-raised exceptions, complementing the existing NumPy `D` convention and preview mode already configured. Enabling the rule surfaced missing sections throughout `lading/` and the test suite. Add NumPy-style `Returns`, `Raises`, and `Yields` sections to the affected functions, fixtures, and helpers, and remove the handful of extraneous `Returns`/`Raises` entries that documented exceptions raised only by delegated calls. Docstrings and the single config line are the only changes; no runtime logic was altered. Relates to #162. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Enable ``ignore-one-line-docstrings`` for pydoclint so the DOC rule group enforces Returns/Raises/Yields completeness on multi-line public docstrings while exempting the concise single-line summaries the project convention uses for private helpers. This reconciles DOC enforcement with the established private-helper docstring style. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Action CodeRabbit review findings for the DOC docstring work: collapse private-helper docstrings to concise single-line summaries (now DOC-exempt via ignore-one-line-docstrings), complete public interface contracts with typed Parameters/Returns/Examples, and correct Raises/Returns wording. Findings that would violate the enforced DOC rules (renaming factory raise-callables to concrete types, documenting delegated/propagated raises) are intentionally not applied. Also fix counter_value to default missing keys to zero, and refactor duplicated E2E/BDD step setup and invocation-recording helpers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Action the follow-up CodeRabbit findings: clarify lockfile empty-tuple docs, add runnable public Examples (cli, config, cmd_mox_runner), complete/collapse docstrings per the public/private convention, remove the create_nontrivial_workspace Examples block (CodeScene Large Method), and consolidate redundant unit-test fixture aliases. Findings that would violate the enforced DOC rules (propagated-exception Raises on transpose_readme_to_crate/optional_mapping/subprocess helpers, factory raise-callable renames) are intentionally skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove only the Examples section from the public ``run`` docstring to resolve CodeScene's "Large Method" diagnostic. The summary, Parameters, and Returns sections are unchanged, and no production logic, signature, delegation boundary (``_dispatch_publication``), or test is touched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the core/utils/app crate-creation block into a new private helper `_create_fixture_crates`, returning the manifest-path mapping, to resolve CodeScene's Large Method diagnostic through a meaningful extraction. Behaviour, dependency fixture text, crate names, and the returned NonTrivialWorkspace are unchanged; the public docstring is untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the misleading `_reject` entry in the NumPy-style Raises headings of expect_mapping, string_mapping, expect_string, boolean, and non_negative_int with `LadingError` — the base of the caller-injected `error` subclass that `_reject` constructs for callers to raise. Each validation-specific failure description is preserved, and the private `_validate_string_pair` helper keeps its one-line docstring. pydoclint (DOC501/DOC502) keys the Raises type token on the syntactic raise-callable (`_reject`) and cannot resolve it to the real exception, so a scoped per-file-ignore for those two rules on the two coercion modules lets the docstrings state the accurate contract, mirroring the existing `ignore-one-line-docstrings` DOC reconciliation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Expand the private helper's docstring with NumPy-style Returns and Raises sections: it returns the validated IPC timeout in seconds and raises CmdMoxError when CMOX_IPC_SOCKET is unset (raised directly) or when CMOX_IPC_TIMEOUT is unparseable, non-finite, or non-positive (propagated from the reachable _resolve_cmd_mox_timeout call). Documentation-only; CmdMoxError is raised directly in the body, so the DOC rules pass without suppression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a NumPy-style Parameters section documenting the make_crate fixture/factory and its role, clarify the Returns section describing the returned plan-building callable, and add a runnable Examples section that invokes the returned factory (via __wrapped__ with a lightweight stand-in) and shows the resulting plan's publishable crate names. Signature and behaviour are unchanged; documentation-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a NumPy-style Returns section for the returned set[Path]: the crate README paths that were written, or reported as would be written during a dry run. Expanding the docstring surfaces the direct re-raise of ReadmeTranspositionError (bare `raise` in the except block), so a matching Raises section is added to satisfy DOC501. Documentation-only; signature and runtime behaviour unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add NumPy-style Parameters sections to the E2EExpectationError factory methods (unsupported_fixture_version, dependency_entry_not_string, args_prefix_mismatch) and to run_cli, extract_dependency_requirement, find_staging_root, and filter_records; add Parameters to given_cargo_commands_stubbed. In extract_dependency_requirement, find_staging_root, and given_nontrivial_workspace_in_git_repo, name the raised type as E2EExpectationError (constructed by the relevant factory classmethod), keeping the factory methods documented as factories. Those callers raise via factory classmethods, so pydoclint keys DOC501/DOC502 on the method name; a scoped per-file-ignore for the two E2E files lets the docstrings state the true E2EExpectationError contract, mirroring the toml_coerce reconciliation. Documentation-only; signatures and runtime behaviour unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add NumPy-style Raises sections documenting the exceptions callers must handle: subprocess_runner (ValueError, CommandSpawnError), invoke_via_subprocess (CommandSpawnError), write_to_sink (OSError, ValueError), transpose_readme_to_crate (ReadmeTranspositionError), and optional_mapping (_reject factory). These functions propagate their exceptions from delegated helpers rather than raising directly, so pydoclint (DOC501/DOC502) — which only tracks direct raises — cannot express the contract. Scope those two rules off for subprocess_runner.py and bump_readme.py (optional_mapping's module already carries the ignore). Documentation-only; behaviour unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8b52666 to
86dddaa
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.
|
@coderabbitai Have the following now been resolved? lading/runtime/subprocess_runner.py (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Coding guidelines tests/unit/publish/test_preflight_lockfile_validation.py (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Path instructions tests/unit/test_bump_lockfiles.py (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Path instructions lading/commands/bump_readme.py (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Path instructions lading/toml_coerce/_mappings.py (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- outlines ---'
ast-grep outline lading/runtime/subprocess_runner.py --items all --view expanded
ast-grep outline tests/unit/publish/test_preflight_lockfile_validation.py --items all --view expanded
ast-grep outline tests/unit/test_bump_lockfiles.py --items all --view expanded
ast-grep outline lading/commands/bump_readme.py --items all --view expanded
ast-grep outline lading/toml_coerce/_mappings.py --items all --view expanded
printf '%s\n' '--- target declarations and surrounding implementation ---'
rg -n -A110 -B12 \
'^def (subprocess_runner|invoke_via_subprocess|write_to_sink|_repository_for_outcomes|_stale_lockfiles_error_message|_inside_dir|_selective_failure_runner|_prepare_manifest_fixture|_assert_failure_message_reports_each_once|transpose_readme_to_crate|optional_mapping|expect_mapping|_spawn_process)\b|^class (CommandSpawnError|ReadmeTranspositionError)\b' \
lading/runtime/subprocess_runner.py \
tests/unit/publish/test_preflight_lockfile_validation.py \
tests/unit/test_bump_lockfiles.py \
lading/commands/bump_readme.py \
lading/toml_coerce/_mappings.py
printf '%s\n' '--- target docstring line counts ---'
python3 - <<'PY'
import ast
from pathlib import Path
targets = {
Path("lading/runtime/subprocess_runner.py"): {
"subprocess_runner", "invoke_via_subprocess", "write_to_sink"
},
Path("tests/unit/publish/test_preflight_lockfile_validation.py"): {
"_repository_for_outcomes", "_stale_lockfiles_error_message"
},
Path("tests/unit/test_bump_lockfiles.py"): {
"_inside_dir", "_selective_failure_runner", "_prepare_manifest_fixture",
"_assert_failure_message_reports_each_once",
},
Path("lading/commands/bump_readme.py"): {"transpose_readme_to_crate"},
Path("lading/toml_coerce/_mappings.py"): {"optional_mapping"},
}
for path, names in targets.items():
tree = ast.parse(path.read_text())
print(path)
for node in ast.walk(tree):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name in names:
doc = ast.get_docstring(node, clean=False)
lines = 0 if doc is None else len(doc.splitlines())
print(f" {node.name}: {lines} docstring line(s); {doc!r}")
PY
printf '%s\n' '--- branch diff for affected files ---'
git diff -- \
lading/runtime/subprocess_runner.py \
tests/unit/publish/test_preflight_lockfile_validation.py \
tests/unit/test_bump_lockfiles.py \
lading/commands/bump_readme.py \
lading/toml_coerce/_mappings.pyLength of output: 93986
Treat this as static current-source verification. The docstring-count utility could not parse |
Replace the factory raise-callable names in NumPy-style Raises headings with the real exception classes: expect_sequence, validate_string_sequence, string_tuple, _validate_matrix_entry, and string_matrix now document LadingError (the base of the injected `error` subclass that `_reject` constructs and the function raises), and _parse_cargo_metadata documents CargoMetadataParseError while preserving the invalid-JSON and non-object-payload conditions. pydoclint keys DOC501/DOC502 on the syntactic raise-callable (`_reject`, the CargoMetadataParseError factory classmethods) and cannot resolve them to the real exception, so a scoped per-file-ignore on the two modules lets the docstrings state the true contract, mirroring the earlier toml_coerce reconciliation. Documentation-only; runtime behaviour unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Our agent can fix these. Install it.
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/publish/test_preflight_cargo_runner.py (1)
118-133: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winKeep this private helper’s docstring to one line.
Replace the expanded Parameters/Returns prose with a concise summary such as “Record the single Cargo preflight command issued for the workspace.”
As per path instructions, private functions and methods must use a single-line summary.
🤖 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 `@tests/unit/publish/test_preflight_cargo_runner.py` around lines 118 - 133, Shorten the private helper’s docstring to a single-line summary describing that it records the single Cargo preflight command issued for the workspace. Remove the expanded implementation details and Returns section from this helper’s docstring.Source: Path instructions
lading/commands/bump_lockfiles.py (1)
29-48: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRestore and correct the public lockfile contracts.
- Add
Raisesand a runnableExamplessection toresolve_lockfile_paths()for invalid paths escaping the workspace or not namingCargo.toml.- Restore
Raisesforregenerate_lockfiles()covering path validation and Cargo/runner failures.- Base aggregation on the number of attempted manifests: one manifest re-raises its original error; multiple manifests produce an aggregate. Do not describe this as “several lockfiles regenerated”.
- Add structured
Raisessections to bothCargoLockfileRepositorydelegation methods.As per coding guidelines and path instructions, public interfaces require complete structured NumPy-style documentation.
Also applies to: 53-100, 265-320
🤖 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_lockfiles.py` around lines 29 - 48, Update the public NumPy-style docstrings for resolve_lockfile_paths, regenerate_lockfiles, and both CargoLockfileRepository delegation methods. Document path-validation errors, including workspace escapes and non-Cargo.toml manifests, and provide a runnable Examples section for resolve_lockfile_paths; document Cargo and runner failures for regenerate_lockfiles. Describe aggregation by attempted manifest count: re-raise the original error for one attempt and aggregate errors for multiple attempts, without referring to regenerated lockfiles.Sources: Coding guidelines, Path instructions
♻️ Duplicate comments (1)
lading/commands/publish.py (1)
223-247: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winComplete the public publish documentation.
- Add
Raisesforprepare_workspace()coveringPublishPreparationErrorfrom unsafe or invalid staging/build paths.- Add
Raisesforrun()coveringPublishPreflightError,PublishPlanError,PublishPreparationError, andPublishErroralong the direct workflow.- Add a representative
Examplessection torun()showing its summary result; use a skipped example for the external workspace and Cargo operations.As per coding guidelines, public interfaces require complete structured NumPy-style documentation with examples and applicable exception contracts.
Also applies to: 661-681
🤖 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/publish.py` around lines 223 - 247, Complete the NumPy-style docstrings for public functions prepare_workspace() and run(). Add a Raises section to prepare_workspace() documenting PublishPreparationError for unsafe or invalid staging/build paths; document run() as raising PublishPreflightError, PublishPlanError, PublishPreparationError, and PublishError during its direct workflow. Add a representative skipped Examples section to run() showing its summary result while avoiding external workspace and Cargo operations.Source: Coding guidelines
🤖 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/cli.py`:
- Around line 353-355: Update the docstring examples for cli.bump and
cli.publish to reflect their delegated string return contracts rather than
asserting the outdated concrete one-line outputs. Use representative assertions
against stable portions of each returned string, covering bump’s formatted
change report and publish’s publication plan plus staging-summary lines, while
keeping the examples clear and executable.
In `@lading/commands/bump_output.py`:
- Around line 182-186: Align the return-value contract in the affected
path-resolution function with its implementation: either update the Returns
documentation to describe the fallback as the original path, or normalize the
out-of-workspace fallback to an absolute path before returning it. Preserve the
existing workspace-relative behavior.
In `@lading/commands/bump.py`:
- Around line 146-150: Update the return documentation for the public function
containing this Returns section to state that the summary includes files changed
or reported as would be changed, covering dry-run previews for manifests,
documents, README files, and lockfiles.
- Around line 307-320: Replace the structured docstring on the private helper
_process_readme_transposition with a single-line summary describing its purpose.
Remove the Returns and Raises sections from this helper; move any caller-facing
exception documentation to the public run function only if needed.
In `@lading/commands/publish_plan.py`:
- Around line 160-186: Complete the public NumPy-style documentation for
plan_publication by annotating its workspace, configuration, and optional
workspace_root parameters with WorkspaceGraph, LadingConfig, and Path | None,
and document PublishPlanError under Raises for invalid ordering or dependency
cycles. Update append_section’s docstring with a structured Returns section
stating that it mutates in place and returns None.
In `@lading/runtime/runner.py`:
- Around line 11-18: Complete the public docstrings at
lading/runtime/runner.py:11-18 for normalise_output with typed value
documentation and a conversion example; at
lading/runtime/subprocess_runner.py:84-87, 185-189, 236-239, 252-263, and
316-319 for subprocess_runner, invoke_via_subprocess, normalise_environment,
relay_stream, and write_to_sink with typed Parameters and concise Examples,
explicitly documenting relay_stream’s None result; add examples for both public
metadata error factories at lading/workspace/metadata.py:73-90; and document the
typed public parameters plus filesystem-transposition example for the relevant
function at lading/commands/bump_readme.py:229-230.
In `@lading/testing/cmd_mox_runner.py`:
- Around line 95-101: Update the structured docstrings for cmd_mox_runner in
lading/testing/cmd_mox_runner.py, the relevant metadata API in
lading/workspace/metadata.py, and the lockfile API in
lading/commands/lockfile.py: add all caller-visible propagated exceptions to
their Raises sections, including CmdMoxError and reachable ValueError cases,
CargoExecutableNotFoundError, CargoMetadataError, CargoMetadataParseError,
CargoMetadataInvocationError, and LockfileDiscoveryError. Keep the existing
Notes distinction for lockfile empty-result paths and remove redundant
exception-only prose where appropriate.
- Around line 130-144: Shorten private helper docstrings to single-line
summaries: reduce _prepare_cmd_mox_context in
lading/testing/cmd_mox_runner.py:130-144; _active_command_runner,
_invoke_cargo_metadata, and _parse_cargo_metadata in
lading/workspace/metadata.py:116-123, 136-149, and 166-178. In
lading/commands/bump_output.py:81-85, 121-126, and 181-186, remove only the
added Returns blocks from _build_changes_description, _format_header, and
_format_manifest_path, preserving the existing CrossHair contract lines.
In `@lading/toml_coerce/_core.py`:
- Around line 23-29: Collapse the private-helper docstrings at
lading/toml_coerce/_core.py:23-29, lading/toml_coerce/_sequences.py:153-165,
lading/workspace/_coercion.py:52-61, and each listed site in
lading/workspace/graph_build.py:108-119, 134-140, 156-162, 189-195, 216-227,
247-258, 269-276, 294-301, 323-330, 345-356, 375-382, 402-408, 425-436, 448-459,
468-480, 496-503, and 516-527 to one concise summary line describing the
specified helper purpose; remove all structured Returns and Raises sections
while preserving the requested meaning for each helper.
In `@lading/toml_coerce/_sequences.py`:
- Around line 42-55: The public docstrings lack complete NumPy-style Parameters
and Examples sections. Update lading/toml_coerce/_sequences.py lines 42-55 to
document every parameter and demonstrate both allow_none outcomes;
lading/config.py lines 437-448 to add a use_configuration example;
lading/toml_coerce/_mappings.py lines 33-35, 66-68, and 116-117 to add the
requested accepted-mapping, None/mapping, and None/validated-table examples;
lading/toml_coerce/_scalars.py lines 28-29, 62-64, and 99-102 to add
accepted/rejected, default/explicit-boolean, and default/string/negative-value
examples; lading/toml_coerce/_sequences.py lines 71-76, 90-101, 116-129, and
185-198 to document the specified parameters and outcomes or supported input
examples; and lading/workspace/graph_build.py lines 40-51 to add a safe skipped
workspace-loading example.
In `@tests/bdd/steps/test_bump_steps.py`:
- Around line 75-93: Complete the NumPy-style public docstrings by adding
runnable Examples showing usage and outcomes for the normal and dry-run bump
helpers in tests/bdd/steps/test_bump_steps.py:75-93 and :104-122, the dependency
assertion step in tests/bdd/steps/test_common_steps.py:132-145, the pytest
fixtures in tests/unit/publish/test_publish_execution_helpers.py:76-88 and
tests/unit/test_bump_lockfiles.py:487-499, and each workspace metadata step in
tests/bdd/steps/test_workspace_metadata_steps.py:29-35, :56-62, :79-85,
:113-119, and :146-152. In the workspace metadata docstrings, add typed
Parameters for tmp_path at :29-35, workspace_directory at :56-62, :79-85, and
:146-152, and all fixture parameters at :113-119; make each example reflect the
helper’s actual invocation and expected result.
In `@tests/bdd/steps/test_publish_fixtures.py`:
- Around line 23-29: Remove the listed Examples sections from
tests/bdd/steps/test_publish_fixtures.py (23-29, 43-51, 78-90),
tests/bdd/steps/test_publish_given_steps.py (248-253, 287-293),
tests/bdd/steps/test_publish_infrastructure.py (91-104),
tests/bdd/steps/test_publish_then_steps.py (117-124, 160-167, 200-207, 240-247,
276-283, 315-322, 368-375, 404-411, 441-448, 484-492),
tests/bdd/steps/test_publish_when_steps.py (43-49, 85-90, 131-137, 173-179), and
tests/unit/test_publish_patch_strategy.py (54-67); retain each surrounding
docstring’s structured contract and all implementation and test logic unchanged.
In `@tests/e2e/conftest.py`:
- Around line 126-133: Extend each new public function docstring with a concise
usage-and-outcome example: tests/e2e/conftest.py:126-133 show invoking
staging_cleanup and verifying removal;
tests/e2e/helpers/e2e_steps_helpers.py:31-90 document the error factories,
:102-108 show the missing-staging-root error, :113-129 add a skipped
integration-style captured-result example, :153-171 cover string and table
dependency entries, :200-217 show parsed staging-root output, and :227-240 show
filtered invocation records; tests/e2e/helpers/workspace_builder.py:32-45 show a
managed temporary directory producing a workspace;
tests/e2e/steps/test_e2e_steps.py:130-143 show skipped pytest-bdd usage and
returned state; tests/unit/conftest.py:178-189 show constructing a workspace
from the factory, :203-216 consuming the composite fixture, :234-240 creating a
dependency record, :255-266 showing the staging-path relationship, and :272-283
showing configured publish options.
In `@tests/e2e/helpers/e2e_steps_helpers.py`:
- Around line 91-97: Update the public factory’s docstring to add NumPy-style
Parameters entries documenting label and args, and include a concise example
demonstrating the missing pre-flight target directory flag and resulting
E2EExpectationError. Keep the existing Returns section and behavior unchanged.
In `@tests/e2e/helpers/git_helpers.py`:
- Around line 84-91: Update the doctest examples for git_init and
git_status_porcelain to use TemporaryDirectory context managers instead of
tempfile.mkdtemp(), ensuring each temporary repository is automatically cleaned
up after execution; apply the same change to both documented examples.
In `@tests/unit/test_cmd_mox_integration.py`:
- Around line 120-123: Replace the multi-line structured docstrings for private
helpers _timeout_cases in tests/unit/test_cmd_mox_integration.py:120-123 and
_failing_runner in tests/unit/test_command_failure_messages.py:41-44 with
concise single-line summaries, preserving each helper’s existing meaning and
behavior.
---
Outside diff comments:
In `@lading/commands/bump_lockfiles.py`:
- Around line 29-48: Update the public NumPy-style docstrings for
resolve_lockfile_paths, regenerate_lockfiles, and both CargoLockfileRepository
delegation methods. Document path-validation errors, including workspace escapes
and non-Cargo.toml manifests, and provide a runnable Examples section for
resolve_lockfile_paths; document Cargo and runner failures for
regenerate_lockfiles. Describe aggregation by attempted manifest count: re-raise
the original error for one attempt and aggregate errors for multiple attempts,
without referring to regenerated lockfiles.
In `@tests/unit/publish/test_preflight_cargo_runner.py`:
- Around line 118-133: Shorten the private helper’s docstring to a single-line
summary describing that it records the single Cargo preflight command issued for
the workspace. Remove the expanded implementation details and Returns section
from this helper’s docstring.
---
Duplicate comments:
In `@lading/commands/publish.py`:
- Around line 223-247: Complete the NumPy-style docstrings for public functions
prepare_workspace() and run(). Add a Raises section to prepare_workspace()
documenting PublishPreparationError for unsafe or invalid staging/build paths;
document run() as raising PublishPreflightError, PublishPlanError,
PublishPreparationError, and PublishError during its direct workflow. Add a
representative skipped Examples section to run() showing its summary result
while avoiding external workspace and Cargo operations.
🪄 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: 2b9776fc-d3eb-4707-8131-20fdd2923313
📒 Files selected for processing (65)
lading/cli.pylading/commands/_shared.pylading/commands/bump.pylading/commands/bump_lockfiles.pylading/commands/bump_manifests.pylading/commands/bump_output.pylading/commands/bump_readme.pylading/commands/bump_toml.pylading/commands/lockfile.pylading/commands/publish.pylading/commands/publish_execution.pylading/commands/publish_index_check.pylading/commands/publish_manifest.pylading/commands/publish_plan.pylading/commands/publish_preflight.pylading/config.pylading/runtime/runner.pylading/runtime/subprocess_runner.pylading/testing/cmd_mox_runner.pylading/testing/toml_utils.pylading/toml_coerce/_core.pylading/toml_coerce/_mappings.pylading/toml_coerce/_scalars.pylading/toml_coerce/_sequences.pylading/utils/metrics.pylading/utils/process.pylading/workspace/_coercion.pylading/workspace/graph_build.pylading/workspace/metadata.pylading/workspace/models.pypyproject.tomltests/bdd/steps/config_fixtures.pytests/bdd/steps/manifest_fixtures.pytests/bdd/steps/test_bump_steps.pytests/bdd/steps/test_commands_catalogue_steps.pytests/bdd/steps/test_common_steps.pytests/bdd/steps/test_publish_fixtures.pytests/bdd/steps/test_publish_given_steps.pytests/bdd/steps/test_publish_helpers.pytests/bdd/steps/test_publish_infrastructure.pytests/bdd/steps/test_publish_then_steps.pytests/bdd/steps/test_publish_when_steps.pytests/bdd/steps/test_workspace_metadata_steps.pytests/conftest.pytests/e2e/conftest.pytests/e2e/helpers/e2e_steps_helpers.pytests/e2e/helpers/git_helpers.pytests/e2e/helpers/workspace_builder.pytests/e2e/steps/test_e2e_steps.pytests/helpers/workspace_metadata.pytests/unit/conftest.pytests/unit/publish/conftest.pytests/unit/publish/preflight_test_utils.pytests/unit/publish/test_cargo_output_adapter.pytests/unit/publish/test_preflight_cargo_runner.pytests/unit/publish/test_preflight_lockfile_validation.pytests/unit/publish/test_publish_execution_helpers.pytests/unit/test_bump_context_properties.pytests/unit/test_bump_lockfiles.pytests/unit/test_bump_rebuild_lockfiles_resolution.pytests/unit/test_cmd_mox_integration.pytests/unit/test_command_failure_messages.pytests/unit/test_publish_patch_strategy.pytests/unit/test_publish_planning.pytests/unit/test_publish_staging.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)
💤 Files with no reviewable changes (1)
- lading/testing/toml_utils.py
| >>> from lading.cli import bump | ||
| >>> print(bump("1.2.3", dry_run=True)) # doctest: +SKIP | ||
| Bumped workspace manifests to 1.2.3 (dry run) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the CLI examples with the delegated return values.
Replace the concrete outputs in Lines [353]-[355] and [413]-[415]. bump() returns the formatted change report produced by commands.bump.run, while publish() returns the publication plan followed by staging-summary lines; neither contract guarantees the shown one-line messages. Use representative assertions against the returned string or document the actual summary shape.
As per coding guidelines, function documentation must include clear examples demonstrating usage and outcome.
Also applies to: 413-415
🤖 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/cli.py` around lines 353 - 355, Update the docstring examples for
cli.bump and cli.publish to reflect their delegated string return contracts
rather than asserting the outdated concrete one-line outputs. Use representative
assertions against stable portions of each returned string, covering bump’s
formatted change report and publish’s publication plan plus staging-summary
lines, while keeping the examples clear and executable.
Source: Coding guidelines
| Returns | ||
| ------- | ||
| str | ||
| The path relative to ``workspace_root``, or the absolute path when it | ||
| lies outside the workspace. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the path contract with the implementation.
The docstring promises an absolute path outside workspace_root, but the function returns str(manifest_path) unchanged; a relative input therefore remains relative. Change the wording to “the original path” or normalize the fallback before returning it.
🤖 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_output.py` around lines 182 - 186, Align the
return-value contract in the affected path-resolution function with its
implementation: either update the Returns documentation to describe the fallback
as the original path, or normalize the out-of-workspace fallback to an absolute
path before returning it. Preserve the existing workspace-relative behavior.
| Returns | ||
| ------- | ||
| str | ||
| Human-readable summary of the manifests, documents, README files, and | ||
| lockfiles that were changed. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe dry-run output as prospective changes.
Change the return description at Line [149] from files that “were changed” to files that “were changed or reported as would be changed”. run() forwards dry_run, so README, documentation, and lockfile paths can represent work that was only previewed.
As per coding guidelines, public interfaces must describe their complete behaviour across supported modes.
🤖 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.py` around lines 146 - 150, Update the return
documentation for the public function containing this Returns section to state
that the summary includes files changed or reported as would be changed,
covering dry-run previews for manifests, documents, README files, and lockfiles.
Source: Coding guidelines
| """Transpose workspace README files into opted-in member crates. | ||
|
|
||
| Returns | ||
| ------- | ||
| set[Path] | ||
| The crate README paths that were written, or that were reported as | ||
| would be written during a dry run. | ||
|
|
||
| Raises | ||
| ------ | ||
| ReadmeTranspositionError | ||
| Propagated from :func:`bump_readme.transpose_readme_to_crate` when a | ||
| crate's README transposition fails; re-raised here after logging. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep the private helper docstring single-line.
Collapse Lines [307]-[320] to one concise summary. _process_readme_transposition is underscore-prefixed, so its structured Returns/Raises block violates the private-helper documentation rule. Move caller-facing exception detail to public run if it must remain documented.
As per path instructions, private functions and methods must use single-line summaries.
🤖 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.py` around lines 307 - 320, Replace the structured
docstring on the private helper _process_readme_transposition with a single-line
summary describing its purpose. Remove the Returns and Raises sections from this
helper; move any caller-facing exception documentation to the public run
function only if needed.
Source: Path instructions
| """Return the :class:`PublishPlan` for ``workspace`` and ``configuration``. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| workspace: | ||
| The resolved workspace graph whose crates are categorised. | ||
| configuration: | ||
| The loaded lading configuration supplying publish exclusions and order. | ||
| workspace_root: | ||
| Optional override for the plan's recorded workspace root. Defaults to | ||
| ``workspace.workspace_root`` when ``None``. | ||
|
|
||
| Returns | ||
| ------- | ||
| PublishPlan | ||
| The resolved plan describing publishable and skipped crate groups. | ||
|
|
||
| Examples | ||
| -------- | ||
| >>> from pathlib import Path | ||
| >>> from lading.config import LadingConfig | ||
| >>> from lading.workspace import WorkspaceGraph | ||
| >>> workspace = WorkspaceGraph(workspace_root=Path("ws"), crates=()) | ||
| >>> plan = plan_publication(workspace, LadingConfig()) | ||
| >>> plan.publishable_names | ||
| () | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Complete the remaining public planning contracts.
- Type the
plan_publication()parameters asWorkspaceGraph,LadingConfig, andPath | None. - Add
RaisesforPublishPlanErrorwhen configured ordering is invalid or dependency cycles prevent planning. - Restore a structured
Returnssection forappend_section()documenting its in-place mutation andNoneresult.
As per coding guidelines and path instructions, public interfaces require complete typed NumPy-style documentation.
Also applies to: 254-277
🤖 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/publish_plan.py` around lines 160 - 186, Complete the public
NumPy-style documentation for plan_publication by annotating its workspace,
configuration, and optional workspace_root parameters with WorkspaceGraph,
LadingConfig, and Path | None, and document PublishPlanError under Raises for
invalid ordering or dependency cycles. Update append_section’s docstring with a
structured Returns section stating that it mutates in place and returns None.
Sources: Coding guidelines, Path instructions
| Examples | ||
| -------- | ||
| >>> from tests.bdd.steps.test_publish_infrastructure import _CommandResponse | ||
| >>> overrides: dict[tuple[str, ...], object] = {} | ||
| >>> overrides["cargo", "test", "--workspace"] = _CommandResponse(exit_code=1) | ||
| >>> overrides["cargo", "test", "--workspace"].exit_code | ||
| 1 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove Examples sections that restate test behaviour.
Keep the structured contracts, but delete these examples: they duplicate the fixture or step implementation rather than documenting an externally consumed API.
tests/bdd/steps/test_publish_fixtures.py#L23-L29: Remove the standalone override-mapping example.tests/bdd/steps/test_publish_fixtures.py#L43-L51: Remove the recorder round-trip example.tests/bdd/steps/test_publish_fixtures.py#L78-L90: Remove the direct context-construction example.tests/bdd/steps/test_publish_given_steps.py#L248-L253: Remove the command-override example.tests/bdd/steps/test_publish_given_steps.py#L287-L293: Remove the skipped workspace-fixture example.tests/bdd/steps/test_publish_infrastructure.py#L91-L104: Remove the direct stub-config example.tests/bdd/steps/test_publish_then_steps.py#L117-L124: Remove the exclusion assertion example.tests/bdd/steps/test_publish_then_steps.py#L160-L167: Remove the pre-flight target assertion example.tests/bdd/steps/test_publish_then_steps.py#L200-L207: Remove the no-exclusions assertion example.tests/bdd/steps/test_publish_then_steps.py#L240-L247: Remove the auxiliary-build assertion example.tests/bdd/steps/test_publish_then_steps.py#L276-L283: Remove the environment assertion example.tests/bdd/steps/test_publish_then_steps.py#L315-L322: Remove the RUSTFLAGS assertion example.tests/bdd/steps/test_publish_then_steps.py#L368-L375: Remove the package-order assertion example.tests/bdd/steps/test_publish_then_steps.py#L404-L411: Remove the dry-run assertion example.tests/bdd/steps/test_publish_then_steps.py#L441-L448: Remove the live-publish assertion example.tests/bdd/steps/test_publish_then_steps.py#L484-L492: Remove the interleaving assertion example.tests/bdd/steps/test_publish_when_steps.py#L43-L49: Remove the skipped publish invocation example.tests/bdd/steps/test_publish_when_steps.py#L85-L90: Remove the invalid-command traceback example.tests/bdd/steps/test_publish_when_steps.py#L131-L137: Remove the skipped forbid-dirty example.tests/bdd/steps/test_publish_when_steps.py#L173-L179: Remove the skipped live-publish example.tests/unit/test_publish_patch_strategy.py#L54-L67: Remove the fixture factory example.
As per coding guidelines, “test documentation should omit examples that merely restate test logic.”
📍 Affects 6 files
tests/bdd/steps/test_publish_fixtures.py#L23-L29(this comment)tests/bdd/steps/test_publish_fixtures.py#L43-L51tests/bdd/steps/test_publish_fixtures.py#L78-L90tests/bdd/steps/test_publish_given_steps.py#L248-L253tests/bdd/steps/test_publish_given_steps.py#L287-L293tests/bdd/steps/test_publish_infrastructure.py#L91-L104tests/bdd/steps/test_publish_then_steps.py#L117-L124tests/bdd/steps/test_publish_then_steps.py#L160-L167tests/bdd/steps/test_publish_then_steps.py#L200-L207tests/bdd/steps/test_publish_then_steps.py#L240-L247tests/bdd/steps/test_publish_then_steps.py#L276-L283tests/bdd/steps/test_publish_then_steps.py#L315-L322tests/bdd/steps/test_publish_then_steps.py#L368-L375tests/bdd/steps/test_publish_then_steps.py#L404-L411tests/bdd/steps/test_publish_then_steps.py#L441-L448tests/bdd/steps/test_publish_then_steps.py#L484-L492tests/bdd/steps/test_publish_when_steps.py#L43-L49tests/bdd/steps/test_publish_when_steps.py#L85-L90tests/bdd/steps/test_publish_when_steps.py#L131-L137tests/bdd/steps/test_publish_when_steps.py#L173-L179tests/unit/test_publish_patch_strategy.py#L54-L67
🤖 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 `@tests/bdd/steps/test_publish_fixtures.py` around lines 23 - 29, Remove the
listed Examples sections from tests/bdd/steps/test_publish_fixtures.py (23-29,
43-51, 78-90), tests/bdd/steps/test_publish_given_steps.py (248-253, 287-293),
tests/bdd/steps/test_publish_infrastructure.py (91-104),
tests/bdd/steps/test_publish_then_steps.py (117-124, 160-167, 200-207, 240-247,
276-283, 315-322, 368-375, 404-411, 441-448, 484-492),
tests/bdd/steps/test_publish_when_steps.py (43-49, 85-90, 131-137, 173-179), and
tests/unit/test_publish_patch_strategy.py (54-67); retain each surrounding
docstring’s structured contract and all implementation and test logic unchanged.
Source: Coding guidelines
| def staging_cleanup() -> cabc.Callable[[Path], None]: | ||
| """Return a helper that removes the publish staging directory parent.""" | ||
| """Return a helper that removes the publish staging directory parent. | ||
|
|
||
| Returns | ||
| ------- | ||
| cabc.Callable[[Path], None] | ||
| A callable that deletes the staging root's parent build directory. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add usage-and-outcome examples to every new public docstring.
tests/e2e/conftest.py#L126-L133: Add an example that invokes the cleanup callable and verifies removal.tests/e2e/helpers/e2e_steps_helpers.py#L31-L90: Add concise examples for the documented error factories.tests/e2e/helpers/e2e_steps_helpers.py#L102-L108: Add an example showing the missing-staging-root error.tests/e2e/helpers/e2e_steps_helpers.py#L113-L129: Add a skipped integration-style example showing the captured result shape.tests/e2e/helpers/e2e_steps_helpers.py#L153-L171: Add examples for string and table dependency entries.tests/e2e/helpers/e2e_steps_helpers.py#L200-L217: Add an example showing parsed staging-root output.tests/e2e/helpers/e2e_steps_helpers.py#L227-L240: Add an example showing filtered invocation records.tests/e2e/helpers/workspace_builder.py#L32-L45: Add a managed-temporary-directory example showing the created workspace.tests/e2e/steps/test_e2e_steps.py#L130-L143: Add a skipped pytest-bdd usage example showing the returned state.tests/unit/conftest.py#L178-L189: Add an example constructing a workspace with the returned factory.tests/unit/conftest.py#L203-L216: Add an example consuming the composite fixture.tests/unit/conftest.py#L234-L240: Add an example creating a dependency record.tests/unit/conftest.py#L255-L266: Add an example showing the staging-path relationship.tests/unit/conftest.py#L272-L283: Add an example showing the configured publish options.
As per coding guidelines, “Function documentation must include clear examples demonstrating usage and outcome.”
📍 Affects 5 files
tests/e2e/conftest.py#L126-L133(this comment)tests/e2e/helpers/e2e_steps_helpers.py#L31-L90tests/e2e/helpers/e2e_steps_helpers.py#L102-L108tests/e2e/helpers/e2e_steps_helpers.py#L113-L129tests/e2e/helpers/e2e_steps_helpers.py#L153-L171tests/e2e/helpers/e2e_steps_helpers.py#L200-L217tests/e2e/helpers/e2e_steps_helpers.py#L227-L240tests/e2e/helpers/workspace_builder.py#L32-L45tests/e2e/steps/test_e2e_steps.py#L130-L143tests/unit/conftest.py#L178-L189tests/unit/conftest.py#L203-L216tests/unit/conftest.py#L234-L240tests/unit/conftest.py#L255-L266tests/unit/conftest.py#L272-L283
🤖 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 `@tests/e2e/conftest.py` around lines 126 - 133, Extend each new public
function docstring with a concise usage-and-outcome example:
tests/e2e/conftest.py:126-133 show invoking staging_cleanup and verifying
removal; tests/e2e/helpers/e2e_steps_helpers.py:31-90 document the error
factories, :102-108 show the missing-staging-root error, :113-129 add a skipped
integration-style captured-result example, :153-171 cover string and table
dependency entries, :200-217 show parsed staging-root output, and :227-240 show
filtered invocation records; tests/e2e/helpers/workspace_builder.py:32-45 show a
managed temporary directory producing a workspace;
tests/e2e/steps/test_e2e_steps.py:130-143 show skipped pytest-bdd usage and
returned state; tests/unit/conftest.py:178-189 show constructing a workspace
from the factory, :203-216 consuming the composite fixture, :234-240 creating a
dependency record, :255-266 showing the staging-path relationship, and :272-283
showing configured publish options.
Source: Coding guidelines
| """Return an error when the pre-flight target dir flag is missing. | ||
|
|
||
| Returns | ||
| ------- | ||
| E2EExpectationError | ||
| The error describing the missing flag. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document both inputs and show the failure outcome.
Add Parameters entries for label and args, plus a concise example. The public factory currently documents only its return value.
As per path instructions, “Docstrings must follow the numpy style guide” and require “full structured docs for all public interfaces.”
🤖 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 `@tests/e2e/helpers/e2e_steps_helpers.py` around lines 91 - 97, Update the
public factory’s docstring to add NumPy-style Parameters entries documenting
label and args, and include a concise example demonstrating the missing
pre-flight target directory flag and resulting E2EExpectationError. Keep the
existing Returns section and behavior unchanged.
Source: Path instructions
| Examples | ||
| -------- | ||
| >>> import tempfile | ||
| >>> from pathlib import Path | ||
| >>> repo = Path(tempfile.mkdtemp()) | ||
| >>> git_init(repo) | ||
| >>> git_status_porcelain(repo) | ||
| '' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use managed temporary directories in the doctests.
Replace tempfile.mkdtemp() with TemporaryDirectory() contexts so executing either example does not leave repositories behind.
As per coding guidelines, “Use context managers … to encapsulate setup and teardown logic for resource management.”
Also applies to: 110-117
🤖 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 `@tests/e2e/helpers/git_helpers.py` around lines 84 - 91, Update the doctest
examples for git_init and git_status_porcelain to use TemporaryDirectory context
managers instead of tempfile.mkdtemp(), ensuring each temporary repository is
automatically cleaned up after execution; apply the same change to both
documented examples.
Source: Coding guidelines
| Returns | ||
| ------- | ||
| st.SearchStrategy[_TimeoutCase] | ||
| A strategy producing timeout cases with their expected outcomes. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore concise private-helper docstrings.
Replace both complete docstrings with single-line summaries; structured Returns sections belong on public interfaces, not _timeout_cases and _failing_runner.
tests/unit/test_cmd_mox_integration.py#L120-L123: Replace the complete_timeout_casesdocstring with a concise summary.tests/unit/test_command_failure_messages.py#L41-L44: Replace the complete_failing_runnerdocstring with a concise summary.
As per coding guidelines, private helpers must use concise summaries; as per path instructions, private functions require single-line summaries.
📍 Affects 2 files
tests/unit/test_cmd_mox_integration.py#L120-L123(this comment)tests/unit/test_command_failure_messages.py#L41-L44
🤖 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 `@tests/unit/test_cmd_mox_integration.py` around lines 120 - 123, Replace the
multi-line structured docstrings for private helpers _timeout_cases in
tests/unit/test_cmd_mox_integration.py:120-123 and _failing_runner in
tests/unit/test_command_failure_messages.py:41-44 with concise single-line
summaries, preserving each helper’s existing meaning and behavior.
Sources: Coding guidelines, Path instructions
2 participants
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