Copilot commented on code in PR #15367:
URL: https://github.com/apache/grails-core/pull/15367#discussion_r2748610497
##########
grails-console/build.gradle:
##########
@@ -41,6 +41,8 @@ dependencies {
api 'org.apache.groovy:groovy-swing'
api 'org.apache.groovy:groovy-groovysh'
implementation 'org.fusesource.jansi:jansi'
+ implementation 'org.jline:jline'
+ // TODO: Remove jline:jline (JLine 2) when upgrading to Groovy 5
(groovy-groovysh 5.x uses JLine 3)
implementation 'jline:jline'
Review Comment:
The dependency should reference the centralized version definition. Instead
of using the literal string 'jline:jline', this should use the reference
defined in dependencies.gradle. For consistency with other dependencies in this
file, this line should be updated to use the jline2 reference from
gradleBomDependencies (e.g., by checking how the platform project dependencies
are structured or using the appropriate reference mechanism).
```suggestion
implementation gradleBomDependencies.jline2
```
##########
grails-bootstrap/src/main/groovy/grails/build/logging/GrailsConsole.java:
##########
@@ -251,51 +240,67 @@ private boolean readPropOrTrue(String prop) {
return property == null ? true : Boolean.valueOf(property);
}
- protected ConsoleReader createConsoleReader(InputStream systemIn) throws
IOException {
- // need to swap out the output to avoid logging during init
- final PrintStream nullOutput = new PrintStream(new
ByteArrayOutputStream());
- final PrintStream originalOut = Log.getOutput();
- try {
- Log.setOutput(nullOutput);
- ConsoleReader consoleReader = new ConsoleReader(systemIn, out);
- consoleReader.setExpandEvents(false);
- return consoleReader;
- } finally {
- Log.setOutput(originalOut);
+ protected LineReader createLineReader(Terminal terminal, History history)
throws IOException {
+ LineReaderBuilder builder = LineReaderBuilder.builder()
+ .terminal(terminal)
+ .option(LineReader.Option.DISABLE_EVENT_EXPANSION, true);
+ if (history != null) {
+ builder.variable(LineReader.HISTORY_FILE, new
File(System.getProperty("user.home"), HISTORYFILE).toPath());
+ builder.history(history);
}
+ return builder.build();
}
/**
- * Creates the instance of Terminal used directly in GrailsConsole. Note
that there is also
- * another terminal instance created implicitly inside of ConsoleReader.
That instance
- * is controlled by the jline.terminal system property.
+ * Creates the instance of Terminal used directly in GrailsConsole.
*/
- protected Terminal createTerminal() {
- terminal = TerminalFactory.create();
- if (isWindows()) {
- terminal.setEchoEnabled(true);
- }
+ protected Terminal createTerminal() throws IOException {
+ Terminal terminal = TerminalBuilder.builder()
+ .system(true)
+ .build();
return terminal;
}
public void resetCompleters() {
- final ConsoleReader reader = getReader();
- if (reader != null) {
- Collection<Completer> completers = reader.getCompleters();
- for (Completer completer : completers) {
- reader.removeCompleter(completer);
- }
+ completers.clear();
+ rebuildLineReader();
+ }
+
+ public void addCompleter(Completer completer) {
+ if (completer != null) {
+ completers.add(completer);
+ rebuildLineReader();
+ }
+ }
+
+ /**
+ * Rebuilds the LineReader with all registered completers using an
AggregateCompleter.
+ * This is necessary because JLine 3 LineReader is immutable once created.
+ */
+ private void rebuildLineReader() {
Review Comment:
Rebuilding the LineReader on every completer addition could have unintended
side effects. Each time a completer is added (via addCompleter), the LineReader
is completely rebuilt, which might reset internal state or cause issues with
command history. Consider building the LineReader only once after all
completers have been registered, or ensure that rebuilding doesn't affect
history or other reader state.
##########
grails-bootstrap/src/main/groovy/grails/build/logging/GrailsConsole.java:
##########
@@ -307,7 +312,7 @@ protected History prepareHistory() throws IOException {
// can't create the file, so no history for you
}
}
- return file.canWrite() ? new FileHistory(file) : null;
+ return file.canWrite() ? new DefaultHistory() : null;
Review Comment:
Verify that history persistence works correctly with DefaultHistory. In
JLine 3, the History object needs to be properly configured with the file path
for persistence. While the HISTORY_FILE variable is set on the
LineReaderBuilder (lines 248, 292), it's important to ensure that the
DefaultHistory instance created here gets properly attached to the file.
Consider testing that command history is actually persisted across sessions, as
the current implementation might not save history to disk correctly.
##########
gradle/docs-dependencies.gradle:
##########
@@ -29,6 +29,7 @@ configurations.register('documentation') {
dependencies {
add('documentation', platform(project(':grails-bom')))
add('documentation', 'org.fusesource.jansi:jansi')
+ // TODO: Remove jline:jline (JLine 2) when upgrading to Groovy 5
(groovy-groovysh 5.x uses JLine 3)
add('documentation', 'jline:jline')
Review Comment:
The dependency should reference the centralized version definition. Instead
of using the literal string 'jline:jline', this should use the jline2 reference
defined in dependencies.gradle for consistency with how other dependencies are
managed in the project.
```suggestion
add('documentation', jline2)
```
##########
grails-gradle/gradle/docs-config.gradle:
##########
@@ -29,6 +29,7 @@ configurations.register('documentation') {
dependencies {
add('documentation', platform(project(':grails-gradle-bom')))
add('documentation', 'org.fusesource.jansi:jansi')
+ // TODO: Remove jline:jline (JLine 2) when upgrading to Groovy 5
(groovy-groovysh 5.x uses JLine 3)
add('documentation', 'jline:jline')
Review Comment:
The dependency should reference the centralized version definition. Instead
of using the literal string 'jline:jline', this should use the jline2 reference
defined in dependencies.gradle for consistency with how other dependencies are
managed in the project.
```suggestion
add('documentation', jline2)
```
--
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]
