Skip to content

refactor: reduce complexity of execute_impl in create_pull_request.rs - #1895

Closed
github-actions[bot] wants to merge 1 commit into
mainfrom
refactor/reduce-complexity-create-pull-request-execute-impl-2-a38139ffff84fbec
Closed

refactor: reduce complexity of execute_impl in create_pull_request.rs#1895
github-actions[bot] wants to merge 1 commit into
mainfrom
refactor/reduce-complexity-create-pull-request-execute-impl-2-a38139ffff84fbec

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

What was complex

CreatePrResult::execute_impl in src/safe_outputs/create_pull_request.rs was flagged by Clippy's too_many_lines lint at 583/100 lines (a previous pass had already reduced it to 362/100). It mixed several independent concerns inline: repository allow-list resolution, patch loading/validation/security checks, git worktree setup, base-commit resolution, and source-branch collision handling — all interleaved with the PR creation logic itself, making the function hard to read and test in isolation.

What changed

Extracted five cohesive blocks into named helper functions, each returning Result<Result<T, ExecutionResult>> (outer for infra errors via anyhow, inner for user-facing failures) so the call sites keep the original early-return-on-failure behavior:

  • resolve_target_repository — repository alias/ID resolution against the allowed-repositories list
  • load_and_validate_patch (+ new PatchValidation struct) — patch existence/size/SHA-256 integrity/path-security/protected-files/max-files checks
  • create_target_worktree — git-repo verification and worktree creation (with origin/ fallback), returning the TempDir, worktree path, and WorktreeGuard
  • resolve_base_commit — recorded (Stage 1) vs. ADO refs API base commit resolution
  • resolve_unique_source_branch — the branch-collision retry loop with random suffix renaming

No public API or observable behavior changes.

Before/after

Lines
Before 583/100 (too_many_lines)
After 349/100 (too_many_lines)

Verification

  • cargo build — clean
  • cargo clippy --all-targets --all-features — clean, no warnings
  • cargo test — full suite passes (2948 passed, 0 failed, 1 ignored — pre-existing)
  • Re-ran the cognitive-complexity/too-many-lines clippy check on the file to confirm the reduction

execute_impl remains above the too_many_lines threshold; further extraction (PR-creation body-building, completion-options/reviewers wiring, file-change collection) is a good next target, tracked in cache memory for the next automated pass.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • spsprodeus21.vssps.visualstudio.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "spsprodeus21.vssps.visualstudio.com"

See Network Configuration for more information.

Generated by Cyclomatic Complexity Reducer · auto · 144.3 AIC · ⌖ 14 AIC · ⊞ 11.1K ·

Extract five more cohesive blocks from CreatePrResult::execute_impl into
named helper functions: resolve_target_repository, load_and_validate_patch,
create_target_worktree, resolve_base_commit, and resolve_unique_source_branch.

No public API or behavior changes. Verified via full test suite (2948
tests passing) and clean clippy.

Before: 583/100 (too_many_lines)
After: 349/100 (too_many_lines)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@jamesadevine

Copy link
Copy Markdown
Collaborator

Closing as superseded by #1946, the strongest and most current version of this create_pull_request decomposition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant