Copilot commented on code in PR #2156: URL: https://github.com/apache/groovy/pull/2156#discussion_r2160059938
########## src/main/java/org/codehaus/groovy/control/ResolveVisitor.java: ########## @@ -95,9 +95,9 @@ */ public class ResolveVisitor extends ClassCodeExpressionTransformer { // note: BigInteger and BigDecimal are also imported by default - // `java.util` is used much frequently than other two java packages(`java.io` and `java.net`), so place java.util before the two packages - public static final String[] DEFAULT_IMPORTS = {"java.lang.", "java.util.", "java.io.", "java.net.", "groovy.lang.", "groovy.util."}; - public static final String[] EMPTY_STRING_ARRAY = new String[0]; + // `java.util` is used much frequently, so place it before `java.io`, et al. Review Comment: [nitpick] The phrase "used much frequently" is awkward; consider revising to "used more frequently" for clearer grammar. ```suggestion // `java.util` is used more frequently, so place it before `java.io`, et al. ``` ########## subprojects/groovy-groovysh/src/test/groovy/org/apache/groovy/groovysh/completion/ImportsSyntaxCompleterTest.groovy: ########## @@ -24,7 +24,9 @@ import org.apache.groovy.groovysh.completion.antlr4.ImportsSyntaxCompleter import static org.apache.groovy.groovysh.completion.TokenUtilTest.tokenList -class ImportsSyntaxCompleterTest extends CompleterTestSupport { +final class ImportsSyntaxCompleterTest extends CompleterTestSupport { + + private static final int DEFAULT_IMPORT_COUNT = org.codehaus.groovy.control.ResolveVisitor.DEFAULT_IMPORTS.size() Review Comment: [nitpick] For readability, import `org.codehaus.groovy.control.ResolveVisitor` at the top and reference `ResolveVisitor.DEFAULT_IMPORTS.size()` instead of the fully qualified name. ########## src/spec/test/PackageTest.groovy: ########## @@ -55,7 +55,8 @@ final class PackageTest { void testDefaultImports() { assertScript ''' // tag::default_import[] - new Date() + long time = LocalDateTime.now().toEpochSecond(ZoneOffset.UTC) + Date date = new Date(time) Review Comment: The `Date(long)` constructor expects milliseconds, but `toEpochSecond` returns seconds. This will produce an incorrect date. Consider using `Instant.now()` with `Date.from(Instant)` or multiplying by 1000. ```suggestion Instant instant = LocalDateTime.now().toInstant(ZoneOffset.UTC) Date date = Date.from(instant) ``` -- 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