Skip to content

refactor: extract shared find_wrapper_executable for Maven/Gradle#1940

Merged
mashraf-222 merged 2 commits intomainfrom
cf-refactor-find-executable
Apr 1, 2026
Merged

refactor: extract shared find_wrapper_executable for Maven/Gradle#1940
mashraf-222 merged 2 commits intomainfrom
cf-refactor-find-executable

Conversation

@HeshamHM28
Copy link
Copy Markdown
Contributor

Summary

  • Extract duplicated parent-directory walking logic from MavenStrategy.find_executable and GradleStrategy.find_executable into a shared find_wrapper_executable() helper in build_tool_strategy.py
  • Both strategies now delegate to a single parameterized function, reducing ~30 lines of near-identical code to 2 one-liner method bodies
  • Maven's find_executable gains parent-directory walking (previously only checked build_root directly), matching Gradle's existing behavior for multi-module projects

Changes

build_tool_strategy.py — Added find_wrapper_executable(build_root, wrapper_names, system_command) that walks up from build_root checking for wrapper scripts at each level, falling back to shutil.which(system_command).

maven_strategy.py — Replaced 12-line find_executable with one-liner delegating to the shared helper.

gradle_strategy.py — Replaced 17-line find_executable with one-liner delegating to the shared helper.

Test plan

  • uv run prek passes (ruff check + ruff format)
  • All 44 tests in test_build_tools.py pass (includes TestMavenExecutable and TestMavenExecutableWithProjectRoot)
  • All 6 tests in test_java_multimodule_deps_install.py pass
  • All 4 tests in test_java_test_filter_validation.py pass

Deduplicate the parent-directory walking logic in MavenStrategy.find_executable
and GradleStrategy.find_executable into a shared find_wrapper_executable() helper
in build_tool_strategy.py. Both strategies now delegate to this single function,
parameterized by wrapper file names and system fallback command.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 1, 2026

Claude finished @HeshamHM28's task in 3m 11s —— View job


PR Review Summary

Prek Checks

All checks passed (ruff check, ruff format). No issues. mypy reports no issues in the 3 changed files.

Code Review

SMALL PR — focused correctness check.

The refactor is clean and correct. A few notes:

Behavioral change in Maven (intentional, documented): The old MavenStrategy.find_executable had CWD-relative fallbacks (Path("mvnw").exists() → returned "./mvnw"). These are gone in the new version. The new implementation walks up from build_root.resolve() instead, which is more robust. This is an improvement — CWD-dependent logic is fragile.

Gradle behavior preserved: The old Gradle loop checked gradlew before gradlew.bat at each level before moving up. The new shared helper does the same (iterates wrapper_names in order at each level). Functionally equivalent.

Minor observation: find_wrapper_executable is an instance method but never uses self — it could be a @staticmethod. Not a bug, just a style note.

No bugs, security issues, or breaking API changes found.

Duplicate Detection

No duplicates detected. The find_wrapper_executable helper is defined once in build_tool_strategy.py and called from both gradle_strategy.py and maven_strategy.py.

Test Coverage

SMALL PR — skipped per review policy.


Last updated: 2026-04-01

The parent-dir walk logic was a standalone function imported by both
strategies. Moving it onto the base class lets children call
self.find_wrapper_executable() with no extra import.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Ubuntu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@HeshamHM28 HeshamHM28 requested a review from mashraf-222 April 1, 2026 07:41
@mashraf-222 mashraf-222 merged commit c077997 into main Apr 1, 2026
35 of 38 checks passed
@mashraf-222 mashraf-222 deleted the cf-refactor-find-executable branch April 1, 2026 19:57
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.

3 participants