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
