Copilot commented on code in PR #2543: URL: https://github.com/apache/groovy/pull/2543#discussion_r3262829704
########## subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/jline/GroovyPrinter.groovy: ########## @@ -0,0 +1,84 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.groovy.groovysh.jline + +import groovy.transform.CompileStatic +import org.jline.builtins.ConfigurationPath +import org.jline.console.Printer +import org.jline.console.ScriptEngine +import org.jline.console.impl.DefaultPrinter + +/** + * A {@link DefaultPrinter} that resolves nanorc highlight-style names + * case-insensitively. + * + * JLine matches the {@code /prnt -s STYLE} value (and the + * {@code valueStyle}/{@code valueStyleAll} options) against the + * {@code syntax "<NAME>"} header of each nanorc grammar with a + * case-sensitive {@code String.equals} (in + * {@code org.jline.builtins.SyntaxHighlighter}), so {@code -s json} + * would not match {@code syntax "JSON"}. All bundled groovysh nanorc + * headers are normalised to UPPERCASE, so normalising the requested + * style to upper case here makes the option accept any casing without + * needing an upstream JLine change. + */ +@CompileStatic +class GroovyPrinter extends DefaultPrinter { + + private static final List<String> STYLE_KEYS = [Printer.STYLE, Printer.VALUE_STYLE, Printer.VALUE_STYLE_ALL] + + GroovyPrinter(ScriptEngine engine, ConfigurationPath configPath) { + super(engine, configPath) + } + + @Override + void println(Map<String, Object> options, Object object) { + super.println(normalizeStyles(options), object) Review Comment: The new case-insensitive style handling is not covered by the existing /prnt tests: PrntTest only exercises the command without -s, and the shared test fixture uses DummyPrinter, so this GroovyPrinter normalization and the production wiring in Main would not be exercised. Please add coverage for non-uppercase /prnt -s input (and, ideally, valueStyle/valueStyleAll defaults) so regressions in the fix are caught. ########## subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/jline/GroovyPrinter.groovy: ########## @@ -0,0 +1,84 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.groovy.groovysh.jline + +import groovy.transform.CompileStatic +import org.jline.builtins.ConfigurationPath +import org.jline.console.Printer +import org.jline.console.ScriptEngine +import org.jline.console.impl.DefaultPrinter + +/** + * A {@link DefaultPrinter} that resolves nanorc highlight-style names + * case-insensitively. + * + * JLine matches the {@code /prnt -s STYLE} value (and the + * {@code valueStyle}/{@code valueStyleAll} options) against the + * {@code syntax "<NAME>"} header of each nanorc grammar with a + * case-sensitive {@code String.equals} (in + * {@code org.jline.builtins.SyntaxHighlighter}), so {@code -s json} + * would not match {@code syntax "JSON"}. All bundled groovysh nanorc + * headers are normalised to UPPERCASE, so normalising the requested + * style to upper case here makes the option accept any casing without + * needing an upstream JLine change. + */ +@CompileStatic +class GroovyPrinter extends DefaultPrinter { + + private static final List<String> STYLE_KEYS = [Printer.STYLE, Printer.VALUE_STYLE, Printer.VALUE_STYLE_ALL] + + GroovyPrinter(ScriptEngine engine, ConfigurationPath configPath) { + super(engine, configPath) + } + + @Override + void println(Map<String, Object> options, Object object) { + super.println(normalizeStyles(options), object) + } + + @Override + protected void highlightAndPrint(Map<String, Object> options, Throwable exception) { + super.highlightAndPrint(normalizeStyles(options), exception) + } + + /** + * Returns a copy of {@code options} with any style-name values upper-cased, + * or the original map untouched when there is nothing to normalise (so an + * immutable or shared caller map is not mutated). + */ + private static Map<String, Object> normalizeStyles(Map<String, Object> options) { + if (options == null || options.isEmpty()) { + return options + } + Map<String, Object> copy = null + for (String key : STYLE_KEYS) { + Object value = options.get(key) + if (value instanceof CharSequence && value.length() > 0) { + String upper = value.toString().toUpperCase(Locale.ROOT) + if (upper != value.toString()) { + if (copy == null) { + copy = new LinkedHashMap<String, Object>(options) + } + copy.put(key, upper) Review Comment: Unconditionally upper-casing the requested style only works with the newly bundled uppercase nanorc headers. DefaultPrinter also resolves user-config nanorc files via ConfigurationPath, and the groovysh docs tell users to copy these files into ~/.groovy; existing copies from previous releases still contain names like "Groovy", "JavaScript", or "sql". Those users can no longer select their existing styles because /prnt -s Groovy is rewritten to GROOVY before JLine performs its case-sensitive lookup. Preserve the original value as a fallback or resolve against the actual configured syntax names case-insensitively instead of forcing uppercase. -- 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]
