Copilot commented on code in PR #760:
URL:
https://github.com/apache/maven-shade-plugin/pull/760#discussion_r3674334566
##########
src/test/java/org/apache/maven/plugins/shade/relocation/SimpleRelocatorTest.java:
##########
@@ -208,6 +208,30 @@ public void testRelocateMavenFiles() {
+ " /** Javadoc, followed by default visibility method with fully
qualified return type */\n"
+ " org.apache.maven.MyReturnType doSomething(
org.apache.maven.Bar bar, org.objectweb.asm.sub.Something something) {\n"
+ " org.apache.maven.Bar bar;\n"
+ + " Map<org.apache.maven.Key, org.apache.maven.Value> map1;\n"
+ + " Map< org.apache.maven.Key, org.apache.maven.Value > map2;\n"
+ + " throw org.apache.maven.Error.newError();\n"
+ + " throw new org.apache.maven.Error();\n"
+ + " boolean flag1 = bar instanceof org.apache.maven.Bar;\n"
+ + " boolean flag2 = org.apache.maven.Utils.yes() ?
org.apache.maven.Utils.one() : org.apache.maven.Utils.zero();\n"
+ + " boolean flag3 = org.apache.maven.Utils.yes() ||
org.apache.maven.Utils.no();\n"
+ + " boolean flag4 = org.apache.maven.Utils.yes() &&
org.apache.maven.Utils.no();\n"
+ + " boolean flag5 = org.apache.maven.Utils.yes() ^^
org.apache.maven.Utils.no();\n"
Review Comment:
`^^` is not a valid Java operator, so this test input is not representative
of real Java sources and may hide/introduce incorrect shade-sources behavior.
If the intent is boolean XOR, use `^`; otherwise replace with a valid boolean
operator consistent with the scenario being tested.
##########
src/test/java/org/apache/maven/plugins/shade/relocation/SimpleRelocatorTest.java:
##########
@@ -241,6 +265,30 @@ public void testRelocateMavenFiles() {
+ " /** Javadoc, followed by default visibility method with fully
qualified return type */\n"
+ " com.acme.maven.MyReturnType doSomething( com.acme.maven.Bar
bar, aj.org.objectweb.asm.sub.Something something) {\n"
+ " com.acme.maven.Bar bar;\n"
+ + " Map<com.acme.maven.Key, com.acme.maven.Value> map1;\n"
+ + " Map< com.acme.maven.Key, com.acme.maven.Value > map2;\n"
+ + " throw com.acme.maven.Error.newError();\n"
+ + " throw new com.acme.maven.Error();\n"
+ + " boolean flag1 = bar instanceof com.acme.maven.Bar;\n"
+ + " boolean flag2 = com.acme.maven.Utils.yes() ?
com.acme.maven.Utils.one() : com.acme.maven.Utils.zero();\n"
+ + " boolean flag3 = com.acme.maven.Utils.yes() ||
com.acme.maven.Utils.no();\n"
+ + " boolean flag4 = com.acme.maven.Utils.yes() &&
com.acme.maven.Utils.no();\n"
+ + " boolean flag5 = com.acme.maven.Utils.yes() ^^
com.acme.maven.Utils.no();\n"
Review Comment:
`^^` is not a valid Java operator, so this test input is not representative
of real Java sources and may hide/introduce incorrect shade-sources behavior.
If the intent is boolean XOR, use `^`; otherwise replace with a valid boolean
operator consistent with the scenario being tested.
##########
src/test/java/org/apache/maven/plugins/shade/relocation/SimpleRelocatorTest.java:
##########
@@ -208,6 +208,30 @@ public void testRelocateMavenFiles() {
+ " /** Javadoc, followed by default visibility method with fully
qualified return type */\n"
+ " org.apache.maven.MyReturnType doSomething(
org.apache.maven.Bar bar, org.objectweb.asm.sub.Something something) {\n"
+ " org.apache.maven.Bar bar;\n"
+ + " Map<org.apache.maven.Key, org.apache.maven.Value> map1;\n"
+ + " Map< org.apache.maven.Key, org.apache.maven.Value > map2;\n"
+ + " throw org.apache.maven.Error.newError();\n"
+ + " throw new org.apache.maven.Error();\n"
+ + " boolean flag1 = bar instanceof org.apache.maven.Bar;\n"
+ + " boolean flag2 = org.apache.maven.Utils.yes() ?
org.apache.maven.Utils.one() : org.apache.maven.Utils.zero();\n"
+ + " boolean flag3 = org.apache.maven.Utils.yes() ||
org.apache.maven.Utils.no();\n"
+ + " boolean flag4 = org.apache.maven.Utils.yes() &&
org.apache.maven.Utils.no();\n"
+ + " boolean flag5 = org.apache.maven.Utils.yes() ^^
org.apache.maven.Utils.no();\n"
+ + " int value1 = org.apache.maven.Utils.x() +
org.apache.maven.Utils.y()\n"
+ + " int value2 = org.apache.maven.Utils.x() -
org.apache.maven.Utils.y()\n"
+ + " int value3 = org.apache.maven.Utils.x() *
org.apache.maven.Utils.y()\n"
+ + " int value4 = org.apache.maven.Utils.x() /
org.apache.maven.Utils.y()\n"
+ + " int value5 = org.apache.maven.Utils.one() |
org.apache.maven.Utils.two()\n"
+ + " int value6 = org.apache.maven.Utils.one() &
org.apache.maven.Utils.two()\n"
+ + " int value7 = org.apache.maven.Utils.one() ^
org.apache.maven.Utils.two()\n"
Review Comment:
These `int valueN = ...` statements are missing semicolons, which makes the
embedded source invalid Java and can undermine the purpose of `shade sources`
tests. Terminate each statement with `;` (and apply the same fix in the
relocated expected-output block at lines 277–283).
##########
src/test/java/org/apache/maven/plugins/shade/relocation/SimpleRelocatorTest.java:
##########
@@ -208,6 +208,30 @@ public void testRelocateMavenFiles() {
+ " /** Javadoc, followed by default visibility method with fully
qualified return type */\n"
+ " org.apache.maven.MyReturnType doSomething(
org.apache.maven.Bar bar, org.objectweb.asm.sub.Something something) {\n"
+ " org.apache.maven.Bar bar;\n"
+ + " Map<org.apache.maven.Key, org.apache.maven.Value> map1;\n"
+ + " Map< org.apache.maven.Key, org.apache.maven.Value > map2;\n"
+ + " throw org.apache.maven.Error.newError();\n"
+ + " throw new org.apache.maven.Error();\n"
+ + " boolean flag1 = bar instanceof org.apache.maven.Bar;\n"
+ + " boolean flag2 = org.apache.maven.Utils.yes() ?
org.apache.maven.Utils.one() : org.apache.maven.Utils.zero();\n"
+ + " boolean flag3 = org.apache.maven.Utils.yes() ||
org.apache.maven.Utils.no();\n"
+ + " boolean flag4 = org.apache.maven.Utils.yes() &&
org.apache.maven.Utils.no();\n"
+ + " boolean flag5 = org.apache.maven.Utils.yes() ^^
org.apache.maven.Utils.no();\n"
+ + " int value1 = org.apache.maven.Utils.x() +
org.apache.maven.Utils.y()\n"
+ + " int value2 = org.apache.maven.Utils.x() -
org.apache.maven.Utils.y()\n"
+ + " int value3 = org.apache.maven.Utils.x() *
org.apache.maven.Utils.y()\n"
+ + " int value4 = org.apache.maven.Utils.x() /
org.apache.maven.Utils.y()\n"
+ + " int value5 = org.apache.maven.Utils.one() |
org.apache.maven.Utils.two()\n"
+ + " int value6 = org.apache.maven.Utils.one() &
org.apache.maven.Utils.two()\n"
+ + " int value7 = org.apache.maven.Utils.one() ^
org.apache.maven.Utils.two()\n"
+ + " switch (org.apache.maven.Utils.getValue()) {\n"
+ + " case org.apache.maven.Utils.STATIC_VALUE:\n"
+ + " org.apache.maven.Utils.info(\"known value\"):\n"
+ + " break;\n"
+ + " default:\n"
+ + " org.apache.maven.Utils.warn(\"unknown value\"):\n"
+ + " break;\n"
Review Comment:
The method calls inside the `switch` block end with `:` rather than `;`,
which is invalid Java syntax. Replace `:` with `;` for these statements (and
apply the same fix in the relocated expected-output block at lines 286 and 289).
##########
src/main/java/org/apache/maven/plugins/shade/relocation/SimpleRelocator.java:
##########
@@ -32,9 +32,13 @@
*/
public class SimpleRelocator implements Relocator {
/**
- * Match dot, slash or space at end of string
+ * Matches <ul>
+ * <li>either dot,</li>
+ * <li>or slash,</li>
+ * <li>or space at the end of a string, where string does NOT end with
operator (generic type, binary, trinary).</li>
+ * </ul>
*/
- private static final Pattern RX_ENDS_WITH_DOT_SLASH_SPACE =
Pattern.compile("[./ ]$");
+ private static final Pattern RX_ENDS_WITH_DOT_SLASH_SPACE =
Pattern.compile("(\\.|/|[^<?:+\\-*/^|&] )$");
Review Comment:
The updated Javadoc says the trailing-space match should not trigger when
the preceding character is an operator including a generic type operator, but
the negated character class does not exclude `>` (which is the common generic
closer before a space, e.g., `List<Foo> bar`). This is inconsistent with the
comment and likely the intended behavior—consider adding `>` to the excluded
set (and adjust the Javadoc if the intent differs).
--
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]
