Hiteshsai007 commented on PR #12418:
URL: https://github.com/apache/maven/pull/12418#issuecomment-4906328390
Thanks for the thorough review! I've gone ahead and addressed all the
feedback.
Here is a summary of the changes made in the latest commit:
1. **Missing edge-case test coverage:**
I rewrote the test using JUnit 5's `@ParameterizedTest` and `@CsvSource`
to effectively cover all the requested edge-cases without duplicating code. The
test now validates:
- User already including the prefix (e.g., `env:MY_VAR`) — verifies no
double-prepending.
- Different prefix types (e.g., `system-property:my_prop`) — verifies the
logic handles arbitrary types beyond `env:`.
- Empty prefixes (`$VAR`) — verifies user input is safely used as-is.
2. **Inline fully-qualified class names in test:**
I removed the inline FQCNs (such as `java.nio.file.Paths`,
`java.util.ArrayList`, `org.jline.consoleui.prompt.ConfirmResult`, etc.) and
correctly imported them at the top of `InitTest.java` to align with the
conventions of the other tests in `impl/maven-cli`.
3. **Replaced anonymous inner classes with lambdas:**
Since `PromptResultItemIF` is structurally a functional interface, I
simplified the code by replacing the anonymous inner classes with lambda
expressions (`() -> result`) in both `Init.java` and the `createResult` helper
in `InitTest.java`.
4. **Removed redundant `template.contains("$")` guard:**
I removed the redundant `if (template.contains("$"))` check inside
`Init.java`. The `editables` list is indeed already strictly filtered by
`e.getValue().getResult().contains("$")` earlier in the pipeline, so removing
the guard safely eliminates unnecessary nesting.
Let me know if there's anything else you'd like me to adjust!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
