Copilot commented on code in PR #2810:
URL: https://github.com/apache/groovy/pull/2810#discussion_r3809063798
##########
src/main/java/groovy/util/ConfigObject.java:
##########
@@ -251,20 +253,20 @@ private void writeConfig(String prefix, ConfigObject map,
BufferedWriter out, in
if (configSize == 1 ||
DefaultGroovyMethods.asBoolean(dotsInKeys)) {
if (firstSize == 1 && firstValue instanceof
ConfigObject) {
- key = KEYWORDS.contains(key) ?
FormatHelper.inspect(key) : key;
+ key = renderKey(key);
String writePrefix = prefix + key + "." + firstKey
+ ".";
writeConfig(writePrefix, (ConfigObject)
firstValue, out, tab, true);
Review Comment:
In the single-entry ConfigObject flattening branch, `firstKey` is appended
into `writePrefix` without being rendered. That means a nested key like `a
b`/`a.b` can still be written as source (and often won’t parse back), even
though other paths now use `renderKey`. Also, if the rendered outer key starts
with a quote and `prefix` is empty, `writePrefix` will start with a string
literal and assignments emitted under it need a receiver (`this.`) to parse.
##########
src/main/java/groovy/util/ConfigObject.java:
##########
@@ -273,30 +275,100 @@ private void writeConfig(String prefix, ConfigObject
map, BufferedWriter out, in
}
}
} else {
- writeValue(key, space, prefix, v, out);
+ writeValue(renderKey(key), space, prefix, v, out);
}
}
}
- private static void writeValue(String key, String space, String prefix,
Object value, BufferedWriter out) throws IOException {
-// key = key.indexOf('.') > -1 ? InvokerHelper.inspect(key) : key;
- boolean isKeyword = KEYWORDS.contains(key);
- key = isKeyword ? FormatHelper.inspect(key) : key;
-
- if (!StringGroovyMethods.asBoolean(prefix) && isKeyword) prefix =
"this.";
-
out.append(space).append(prefix).append(key).append('=').append(FormatHelper.inspect(value));
+ /**
+ * Writes one entry, given a key path whose components have already been
rendered.
+ *
+ * @param keyPath the rendered key path, such as {@code foo} or {@code
foo.'a b'}
+ */
+ private static void writeValue(String keyPath, String space, String
prefix, Object value, BufferedWriter out) throws IOException {
+ // A quoted key cannot open a statement on its own, so it needs a
receiver, exactly as a
+ // keyword key has always done.
+ if (!StringGroovyMethods.asBoolean(prefix) && keyPath.startsWith("'"))
prefix = "this.";
+
out.append(space).append(prefix).append(keyPath).append('=').append(renderValue(value));
out.newLine();
}
private void writeNode(String key, String space, int tab, ConfigObject
value, BufferedWriter out) throws IOException {
- key = KEYWORDS.contains(key) ? FormatHelper.inspect(key) : key;
- out.append(space).append(key).append(" {");
+ out.append(space).append(renderKey(key)).append(" {");
out.newLine();
writeConfig("", value, out, tab + 1, true);
out.append(space).append('}');
out.newLine();
}
+ /**
+ * Renders a key as it must appear in the written configuration: bare when
it is a plain
+ * identifier, and as a quoted literal otherwise. A key which is not an
identifier would
+ * otherwise be written as though it were source, and read back as
whatever it happened to
+ * parse as.
+ *
+ * @param key the key to render
+ * @return the key as it should be written
+ */
+ private static String renderKey(String key) {
+ return isIdentifier(key) ? key : FormatHelper.inspect(key);
+ }
+
+ private static boolean isIdentifier(String key) {
+ if (key == null || key.isEmpty() || KEYWORDS.contains(key)) return
false;
+ if (!Character.isJavaIdentifierStart(key.charAt(0))) return false;
+ for (int i = 1, n = key.length(); i < n; i += 1) {
+ if (!Character.isJavaIdentifierPart(key.charAt(i))) return false;
+ }
+ return true;
+ }
+
+ /**
+ * Renders a value as a literal which reads back as the same data.
+ *
+ * @param value the value to render
+ * @return the value as it should be written
+ */
+ private static String renderValue(Object value) {
+ return FormatHelper.inspect(asWritableData(value));
+ }
+
+ /**
+ * Converts a value into something {@link FormatHelper#inspect} renders as
inert data.
+ * <p>
+ * A {@link CharSequence} which is not a {@code String} is rendered as a
double quoted
+ * literal, in which a dollar is live, so its text is carried over to a
{@code String} and
+ * rendered single quoted instead. A value of any other type without a
literal form would be
+ * written as a bare {@code toString()}, which is not data at all, so its
text is carried
+ * over in the same way. Numbers and booleans already write as themselves.
+ *
+ * @param value the value to convert
+ * @return a value whose rendering is data
+ */
+ private static Object asWritableData(Object value) {
+ if (value == null || value instanceof String || value instanceof
Number || value instanceof Boolean) {
+ return value;
+ }
+ if (value instanceof CharSequence) {
+ return value.toString();
+ }
+ if (value instanceof Map<?, ?> map) {
+ Map<Object, Object> converted = new LinkedHashMap<>(map.size());
+ for (Map.Entry<?, ?> entry : map.entrySet()) {
+ converted.put(asWritableData(entry.getKey()),
asWritableData(entry.getValue()));
+ }
+ return converted;
+ }
+ if (value instanceof Collection<?> collection) {
+ List<Object> converted = new ArrayList<>(collection.size());
+ for (Object element : collection) {
+ converted.add(asWritableData(element));
+ }
+ return converted;
+ }
+ return value.toString();
+ }
Review Comment:
`asWritableData` currently treats Java arrays as “other type” and falls back
to `value.toString()`. For arrays this is the JVM identity form (e.g.
`[Ljava.lang.String;@...` / `[I@...`), which loses the actual elements and
changes `writeTo` behavior vs `FormatHelper.inspect(array)` (which renders a
list-like literal). Consider converting arrays to a `List` and recursively
sanitizing their elements, similar to the `Collection` branch.
--
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]
