Copilot commented on code in PR #2228:
URL: https://github.com/apache/groovy/pull/2228#discussion_r2096073231
##########
subprojects/groovy-groovydoc/src/test/groovy/org/codehaus/groovy/tools/groovydoc/GroovyDocToolTest.java:
##########
@@ -18,13 +18,18 @@
*/
package org.codehaus.groovy.tools.groovydoc;
+import com.github.javaparser.ParserConfiguration;
+import com.github.javaparser.StaticJavaParser;
import groovy.test.GroovyTestCase;
+import org.apache.tools.ant.taskdefs.condition.JavaVersion;
Review Comment:
This import isn't used anywhere in the test class; please remove it to keep
imports clean.
```suggestion
```
##########
subprojects/groovy-ant/src/main/java/org/codehaus/groovy/ant/Groovydoc.java:
##########
@@ -144,6 +145,15 @@ public void setNoVersionStamp(boolean noVersionStamp) {
this.noVersionStamp = noVersionStamp;
}
+ /**
+ * Defaults to a popular Java version. Otherwise, override here for a
specific expected source file version.
+ *
+ * @param javaVersion the expected source level of any Java files that may
be parsed
+ */
+ public void setJavaVersion(String javaVersion) {
Review Comment:
The Javadoc mentions a 'popular Java version' but the code doesn't set a
default. Clarify what default is used when this setter is not called and which
version strings are valid.
##########
subprojects/groovy-groovydoc/src/test/groovy/org/codehaus/groovy/tools/groovydoc/GroovyDocToolTest.java:
##########
@@ -1026,6 +1040,63 @@ public void testClassDeclarationHeader() throws
Exception {
assertTrue("The Groovy derived class declaration header should match
in:\n" + groovyDerivedClass, derivedClass.matcher(groovyDerivedClass).find());
}
+ public void testLanguageLevelNotSupported() throws Exception {
+ htmlTool = makeHtmltool(new ArrayList<LinkArgument>(),
ParserConfiguration.LanguageLevel.JAVA_1_4.name(), new Properties());
+
+ final String base =
"org/codehaus/groovy/tools/groovydoc/testfiles/generics";
+
+ PrintStream originalSystemErr = System.err;
+ ByteArrayOutputStream systemErr = new ByteArrayOutputStream();
+ try {
+ System.setErr(new PrintStream(systemErr));
+ htmlTool.add(Arrays.asList(
+ base + "/Java.java"
+ ));
+ }
+ finally {
+ System.setErr(originalSystemErr);
+ }
+ final String errorMessage = systemErr.toString();
+ System.err.println(errorMessage);
+
+ final MockOutputTool output = new MockOutputTool();
+ htmlTool.renderToOutput(output, MOCK_DIR);
+
+ final String javadoc = output.getText(MOCK_DIR + "/" + base +
"/Java.html");
+ assertNull("Javadoc should be null since language level is not
supported", javadoc);
+
+ assertTrue("Expected to find Java file in error message",
errorMessage.contains("org/codehaus/groovy/tools/groovydoc/testfiles/generics/Java.java"));
+ assertTrue("Expected to find language level not supported",
errorMessage.contains("Pay attention that this feature is supported starting
from"));
Review Comment:
The test expects a specific guidance message, but no such message is emitted
by the code under test. Either adjust the test or output the expected message
when an unsupported level is encountered.
##########
subprojects/groovy-groovydoc/src/main/java/org/codehaus/groovy/tools/groovydoc/GroovyDocTool.java:
##########
@@ -71,12 +77,41 @@ public GroovyDocTool(ResourceManager resourceManager,
String[] sourcepaths, Stri
}
}
+ private ParserConfiguration.LanguageLevel calculateLanguageLevel(String
javaVersion) {
+ String version = Optional.ofNullable(javaVersion)
+ .map(String::trim)
+ .map(s -> s.toUpperCase())
+ .filter(s -> !s.isEmpty())
+ .orElse(null);
+
+ if (version == null) {
+ return null;
+ }
+
+ try {
+ return ParserConfiguration.LanguageLevel.valueOf(version);
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException("Unsupported Java Version: " +
javaVersion);
+ }
+ }
+
public void add(List<String> filenames) throws IOException {
if (templateEngine != null) {
// only print out if we are being used for template generation
log.debug("Loading source files for " + filenames);
}
- rootDocBuilder.buildTree(filenames);
+
+ try {
+ if(javaLanguageLevel != null) {
+
StaticJavaParser.getParserConfiguration().setLanguageLevel(javaLanguageLevel);
+ }
+ rootDocBuilder.buildTree(filenames);
+ }
+ finally {
+ if(javaLanguageLevel != null) {
+
StaticJavaParser.getParserConfiguration().setLanguageLevel(null);
+ }
Review Comment:
You reset the global parser level to null after parsing, but you don't
restore the previous setting. Consider saving the old level before setting and
restoring it in the finally block to avoid side effects in multi-threaded
contexts.
```suggestion
ParserConfiguration.LanguageLevel previousLanguageLevel =
StaticJavaParser.getParserConfiguration().getLanguageLevel();
if (javaLanguageLevel != null) {
StaticJavaParser.getParserConfiguration().setLanguageLevel(javaLanguageLevel);
}
rootDocBuilder.buildTree(filenames);
}
finally {
StaticJavaParser.getParserConfiguration().setLanguageLevel(previousLanguageLevel);
```
--
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: notifications-unsubscr...@groovy.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
