chore: address PR feedback

Signed-off-by: behnazh-w <[email protected]>

Author: behnazh-w

src/macaron/build_spec_generator/build_command_patcher.py

@@ -73,7 +73,7 @@ def _patch_commands(
         try:
             new_cli_command = effective_cli_parser.apply_patch(
                 cli_command=cli_command,
-                options_patch=patch,
+                patch_options=patch,
             )
         except PatchBuildCommandError as error:
             logger.error(

src/macaron/build_spec_generator/cli_command_parser/__init__.py

@@ -114,7 +114,16 @@ def to_cmds(self) -> list[str]:
         """Return the CLI Command as a list of strings."""
 
 
+# T is a generic type variable restricted to subclasses of CLICommand.
+# It ensures that only derived types of CLICommand can be used with
+# generic classes or functions parameterized by T.
 T = TypeVar("T", bound="CLICommand")
+
+# Y_contra is a contravariant type variable intended for CLI argument
+# patch values. Using contravariance allows generic classes or functions
+# to accept supertypes of the specified type parameter, making it easier
+# to support broader value types when implementing patching for different
+# build tools.
 Y_contra = TypeVar("Y_contra", contravariant=True)
 
 
@@ -160,6 +169,6 @@ def is_build_tool(self, executable_path: str) -> bool:
     def apply_patch(
         self,
         cli_command: T,
-        options_patch: Mapping[str, Y_contra | None],
+        patch_options: Mapping[str, Y_contra | None],
     ) -> T:
         """Return a new CLICommand object with its option patched, while persisting the executable path."""

src/macaron/build_spec_generator/cli_command_parser/gradle_cli_command.py

@@ -382,7 +382,4 @@ class GradleCLICommand:
 
     def to_cmds(self) -> list[str]:
         """Return the CLI Command as a list of strings."""
-        result = []
-        result.append(self.executable)
-        result.extend(self.options.to_option_cmds())
-        return result
+        return [self.executable] + self.options.to_option_cmds()

src/macaron/build_spec_generator/cli_command_parser/gradle_cli_parser.py

@@ -52,9 +52,7 @@ def is_valid_patch_option(self, patch: Any) -> TypeGuard[bool]:
 
     def add_to_arg_parser(self, arg_parse: argparse.ArgumentParser) -> None:
         """Add a new argument to argparser.ArgumentParser representing this option."""
-        kwargs: dict[str, Any] = {}
-
-        kwargs["action"] = "store_true"
+        kwargs: dict[str, Any] = {"action": "store_true"}
         if self.dest:
             kwargs["dest"] = self.dest
 
@@ -75,7 +73,7 @@ def get_patch_type_str(self) -> str:
 
 
 @dataclass
-class GradleOptionalNegateableFlag(OptionDef[bool]):
+class GradleOptionalNegatableFlag(OptionDef[bool]):
     """This option represents an optional negatable flag in Gradle CLI command.
 
     For example: --build-cache/--no-build-cache
@@ -144,7 +142,7 @@ def get_patch_type_str(self) -> str:
 
 
 @dataclass
-class GradlePropeties(OptionDef[dict[str, str | None]]):
+class GradleProperties(OptionDef[dict[str, str | None]]):
     """This option represents an option used to define property values of a Gradle CLI command.
 
     This option can be defined multiple times and the values are appended into a list of string in argparse.
@@ -175,7 +173,7 @@ def get_patch_type_str(self) -> str:
 
 @dataclass
 class GradleTask(OptionDef[list[str]]):
-    """This option represents the positional task option in Maven CLI command.
+    """This option represents the positional task option in Gradle CLI command.
 
     argparse.Namespace stores this as a list of string. This is stored internally as a list of string.
     """
@@ -314,25 +312,25 @@ def get_patch_type_str(self) -> str:
         short_names=None,
         long_name="--write-locks",
     ),
-    GradleOptionalNegateableFlag(
+    GradleOptionalNegatableFlag(
         long_name="--build-cache",
     ),
-    GradleOptionalNegateableFlag(
+    GradleOptionalNegatableFlag(
         long_name="--configuration-cache",
     ),
-    GradleOptionalNegateableFlag(
+    GradleOptionalNegatableFlag(
         long_name="--configure-on-demand",
     ),
-    GradleOptionalNegateableFlag(
+    GradleOptionalNegatableFlag(
         long_name="--daemon",
     ),
-    GradleOptionalNegateableFlag(
+    GradleOptionalNegatableFlag(
         long_name="--parallel",
     ),
-    GradleOptionalNegateableFlag(
+    GradleOptionalNegatableFlag(
         long_name="--scan",
     ),
-    GradleOptionalNegateableFlag(
+    GradleOptionalNegatableFlag(
         long_name="--watch-fs",
     ),
     # This has been validated by setting up a minimal gradle project. Gradle version 8.14.2
@@ -430,11 +428,11 @@ def get_patch_type_str(self) -> str:
         short_name=None,
         long_name="--warning-mode",
     ),
-    GradlePropeties(
+    GradleProperties(
         short_name="-D",
         long_name="--system-prop",
     ),
-    GradlePropeties(
+    GradleProperties(
         short_name="-P",
         long_name="--project-prop",
     ),
@@ -447,7 +445,7 @@ def get_patch_type_str(self) -> str:
 class GradleCLICommandParser:
     """A Gradle CLI Command Parser."""
 
-    ACCEPTABLE_EXECUTABLE = ["gradle", "gradlew"]
+    ACCEPTABLE_EXECUTABLE = {"gradle", "gradlew"}
 
     def __init__(self) -> None:
         """Initialize the instance."""
@@ -561,8 +559,35 @@ def _patch_properties_mapping(
         option_long_name: str,
         patch_value: GradleOptionPatchValueType,
     ) -> dict[str, str]:
+        """
+        Apply a patch value to an existing properties dictionary for a specified Gradle option.
+
+        This function locates the metadata definition for the given option by its long name,
+        ensures it is a properties-type option, validates the patch value type, and then
+        applies the patch using `patch_mapping`. Throws a `PatchBuildCommandError` if the
+        option is not valid or the patch value's type is incorrect.
+
+        Parameters
+        ----------
+        original_props: dict[str, str]
+            The original mapping of property names to values.
+        option_long_name: str
+            The long name of the Gradle option to patch.
+        patch_value: GradleOptionPatchValueType
+            The patch to apply to the properties mapping.
+
+        Returns
+        -------
+        dict[str, str]
+            The updated properties mapping after applying the patch.
+
+        Raises
+        ------
+        PatchBuildCommandError
+            If the option is not a valid properties-type option or the patch value does not have a valid type.
+        """
         prop_opt_def = self.option_defs.get(option_long_name)
-        if not prop_opt_def or not isinstance(prop_opt_def, GradlePropeties):
+        if not prop_opt_def or not isinstance(prop_opt_def, GradleProperties):
             raise PatchBuildCommandError(f"{option_long_name} from the patch is not a property type option.")
 
         if not prop_opt_def.is_valid_patch_option(patch_value):
@@ -578,11 +603,11 @@ def _patch_properties_mapping(
     def apply_patch(
         self,
         cli_command: GradleCLICommand,
-        options_patch: Mapping[str, GradleOptionPatchValueType | None],
+        patch_options: Mapping[str, GradleOptionPatchValueType | None],
     ) -> GradleCLICommand:
         """Patch the options of a Gradle CLI command, while persisting the executable path.
 
-        `options_patch` is a mapping with:
+        `patch_options` is a mapping with:
 
         - **Key**: the long name of a Gradle CLI option as string. For example: ``--continue``, ``--build-cache``.
           For patching tasks, use the key ``tasks``.
@@ -630,7 +655,7 @@ def apply_patch(
             executable=cli_command.executable,
             options=self.apply_option_patch(
                 cli_command.options,
-                patch=options_patch,
+                patch=patch_options,
             ),
         )
 

src/macaron/build_spec_generator/cli_command_parser/maven_cli_command.py

@@ -318,7 +318,4 @@ class MavenCLICommand:
 
     def to_cmds(self) -> list[str]:
         """Return the CLI Command as a list of strings."""
-        result = []
-        result.append(self.executable)
-        result.extend(self.options.to_option_cmds())
-        return result
+        return [self.executable] + self.options.to_option_cmds()

src/macaron/build_spec_generator/cli_command_parser/maven_cli_parser.py

@@ -123,7 +123,7 @@ def get_patch_type_str(self) -> str:
 
 
 @dataclass
-class MavenSystemPropeties(OptionDef[dict[str, str | None]]):
+class MavenSystemProperties(OptionDef[dict[str, str | None]]):
     """This option represents the -D/--define option of a Maven CLI command.
 
     This option can be defined multiple times and the values are appended into a list of string in argparse.
@@ -289,7 +289,7 @@ def get_patch_type_str(self) -> str:
         short_name="-b",
         long_name="--builder",
     ),
-    MavenSystemPropeties(
+    MavenSystemProperties(
         short_name="-D",
         long_name="--define",
     ),
@@ -347,7 +347,7 @@ def get_patch_type_str(self) -> str:
 class MavenCLICommandParser:
     """A Maven CLI Command Parser."""
 
-    ACCEPTABLE_EXECUTABLE = ["mvn", "mvnw"]
+    ACCEPTABLE_EXECUTABLE = {"mvn", "mvnw"}
 
     def __init__(self) -> None:
         """Initialize the instance."""
@@ -446,16 +446,6 @@ def parse(self, cmd_list: list[str]) -> "MavenCLICommand":
         except SystemExit as sys_exit_err:
             raise CommandLineParseError(f"Failed to parse the Maven CLI Options {' '.join(options)}.") from sys_exit_err
 
-        # Handle cases where goal or plugin phase is not provided.
-        if not parsed_opts.goals:
-            # Allow cases such as:
-            #   mvn --help
-            #   mvn --version
-            # Note that we don't allow mvn -V or mvn --show-version as this command will
-            #   fail for mvn.
-            if not parsed_opts.help_ and not parsed_opts.version:
-                raise CommandLineParseError(f"No goal detected for {' '.join(options)}.")
-
         maven_cli_options = MavenCLIOptions.from_parsed_arg(parsed_opts)
 
         return MavenCLICommand(
@@ -469,8 +459,36 @@ def _patch_properties_mapping(
         option_long_name: str,
         patch_value: MavenOptionPatchValueType,
     ) -> dict[str, str]:
+        """
+        Apply a patch to the Maven system properties mapping for a specific option.
+
+        Retrieves the system property option definition for the specified long name,
+        validates its type and the patch value, and applies the patch update to the
+        original properties dictionary. Raises an error if the option or patch type
+        is invalid for Maven `--define` options.
+
+        Parameters
+        ----------
+        original_props : dict[str, str]
+            The original dictionary of Maven system property names and their values.
+        option_long_name : str
+            The long name of the Maven option to patch (usually '--define').
+        patch_value : MavenOptionPatchValueType
+            The value to patch into the original properties dictionary.
+
+        Returns
+        -------
+        dict[str, str]
+            The updated mapping with the patch applied.
+
+        Raises
+        ------
+        PatchBuildCommandError
+            If the option is not a Maven system property option or if the patch value
+            has an invalid type.
+        """
         define_opt_def = self.option_defs.get(option_long_name)
-        if not define_opt_def or not isinstance(define_opt_def, MavenSystemPropeties):
+        if not define_opt_def or not isinstance(define_opt_def, MavenSystemProperties):
             raise PatchBuildCommandError(f"{option_long_name} from the patch is not a --define option.")
 
         if not define_opt_def.is_valid_patch_option(patch_value):
@@ -484,11 +502,11 @@ def _patch_properties_mapping(
     def apply_patch(
         self,
         cli_command: MavenCLICommand,
-        options_patch: Mapping[str, MavenOptionPatchValueType | None],
+        patch_options: Mapping[str, MavenOptionPatchValueType | None],
     ) -> MavenCLICommand:
         """Patch the options of a Gradle CLI command, while persisting the executable path.
 
-        `options_patch` is a mapping with:
+        `patch_options` is a mapping with:
 
         - **Key**: the long name of a Maven CLI option as a string. For example: ``--define``, ``--settings``.
           For patching goals or plugin phases, use the key `goals` with the value being a list of strings.
@@ -529,7 +547,7 @@ def apply_patch(
             executable=cli_command.executable,
             options=self.apply_option_patch(
                 cli_command.options,
-                patch=options_patch,
+                patch=patch_options,
             ),
         )
 

src/macaron/build_spec_generator/jdk_finder.py

@@ -166,8 +166,6 @@ def find_jdk_version_from_remote_maven_repo_standalone(
         The version part of the GAV coordinate.
     asset_name: str
         The name of artifact to download and extract the jdk version.
-    ext: JavaArtifactExt
-        The extension of the main artifact file.
     remote_maven_repo_url: str
         The URL to the remote maven layout repository.