Copilot commented on code in PR #397:
URL: https://github.com/apache/commons-jexl/pull/397#discussion_r2763580466
##########
src/test/java/org/apache/commons/jexl3/Issues400Test.java:
##########
@@ -798,6 +799,38 @@ void testIssue441() {
Assertions.assertEquals(ctl, o);
}
+ @Test
+ void testIssue455a() {
+ final JexlEngine jexl = new JexlBuilder().create();
+ String code = "name -> `${name +\n name}`";
+ JexlScript script = jexl.createScript(code);
+ Object o = script.execute(null, "Hello");
+ String ctl = "HelloHello";
+ Assertions.assertEquals(ctl, o);
+ }
+
+ @Test
+ void testIssue455b() {
+ final JexlEngine jexl = new JexlBuilder().create();
+ String code = "name -> `${name}\n${name}`;";
+ JexlScript script = jexl.createScript(code);
+ Object o = script.execute(null, "Hello");
+ String ctl = "Hello\nHello";
+ Assertions.assertEquals(ctl, o);
+ }
+
+ @Test
+ void testIssue455() {
+ final JexlEngine jexl = new JexlBuilder().create();
+ final JexlContext context = new MapContext();
+ context.set("name", "Hello");
+ final JxltEngine jxlt = jexl.createJxltEngine();
+ final JxltEngine.Template template =
jxlt.createTemplate("<b>\n\t${name\n\t+ name}\n</b>");
+ final StringWriter writer = new StringWriter();
+ template.evaluate(context, writer);
+ assertEquals("<b>\n\tHelloHello\n</b>", writer.toString());
Review Comment:
Inconsistent use of `assertEquals`. While the static import allows this
usage, the surrounding test methods (testIssue455a at line 809, testIssue455b
at line 819, and testIssue442 at line 843) consistently use
`Assertions.assertEquals()` instead. For consistency with the nearby code,
consider changing to `Assertions.assertEquals()`.
```suggestion
Assertions.assertEquals("<b>\n\tHelloHello\n</b>",
writer.toString());
```
##########
src/main/java/org/apache/commons/jexl3/internal/TemplateEngine.java:
##########
@@ -1086,54 +1086,54 @@ TemplateExpression parseExpression(final JexlInfo info,
final String expr, final
// nested immediate in deferred; need to balance count of '{'
& '}'
// closing '}'
switch (c) {
- case '"':
- case '\'':
- strb.append(c);
- column = StringParser.readString(strb, expr, column + 1,
c);
- continue;
- case '{':
- if (expr.charAt(column - 1) == immediateChar) {
- inner1 += 1;
- strb.deleteCharAt(strb.length() - 1);
- nested = true;
- } else {
- deferred1 += 1;
- strb.append(c);
- }
- continue;
- case '}':
- // balance nested immediate
- if (deferred1 > 0) {
- deferred1 -= 1;
+ case '"':
+ case '\'':
strb.append(c);
- } else if (inner1 > 0) {
- inner1 -= 1;
- } else {
- // materialize the nested/deferred expr
- final String src = escapeString(strb);
- final JexlInfo srcInfo = info.at(lineno, column);
- TemplateExpression dexpr;
- if (nested) {
- dexpr = new NestedExpression(
- escapeString(expr.substring(inested,
column + 1)),
+ column = StringParser.readString(strb, expr, column +
1, c);
+ continue;
+ case '{':
+ if (expr.charAt(column - 1) == immediateChar) {
+ inner1 += 1;
+ strb.deleteCharAt(strb.length() - 1);
+ nested = true;
+ } else {
+ deferred1 += 1;
+ strb.append(c);
+ }
+ continue;
+ case '}':
+ // balance nested immediate
+ if (deferred1 > 0) {
+ deferred1 -= 1;
+ strb.append(c);
+ } else if (inner1 > 0) {
+ inner1 -= 1;
+ } else if (!isIgnorable(c)) {
+ // materialize the nested/deferred expr
+ final String src = escapeString(strb);
+ final JexlInfo srcInfo = info.at(lineno, column);
+ TemplateExpression dexpr;
+ if (nested) {
+ dexpr = new NestedExpression(
+
escapeString(expr.substring(inested, column + 1)),
+ jexl.jxltParse(srcInfo, noscript,
src, scope),
+ null);
+ } else {
+ dexpr = new DeferredExpression(
+ src,
jexl.jxltParse(srcInfo, noscript, src,
scope),
null);
- } else {
- dexpr = new DeferredExpression(
- src,
- jexl.jxltParse(srcInfo, noscript, src,
scope),
- null);
+ }
+ builder.add(dexpr);
+ strb.delete(0, Integer.MAX_VALUE);
+ nested = false;
+ state = ParseState.CONST;
}
- builder.add(dexpr);
- strb.delete(0, Integer.MAX_VALUE);
- nested = false;
- state = ParseState.CONST;
- }
- break;
- default:
- // do buildup expr
- column = append(strb, expr, column, c);
- break;
+ break;
+ default:
+ // do buildup expr
+ column = append(strb, expr, column, c);
Review Comment:
The default case in DEFERRED1 unconditionally appends all characters to the
expression, including ignorable whitespace characters like newlines and tabs.
This is inconsistent with the IMMEDIATE1 case (line 1076) which filters out
ignorable characters.
For consistency with the immediate expression handling and based on the PR's
goal to ignore whitespace in embedded expressions, the default case should
filter ignorable characters before appending. Consider changing line 1135 to:
- Only call append if the character is not ignorable
- Add a condition similar to the IMMEDIATE1 case: `if (!isIgnorable(c)) {
column = append(strb, expr, column, c); }`
```suggestion
if (!isIgnorable(c)) {
column = append(strb, expr, column, c);
}
```
##########
src/main/java/org/apache/commons/jexl3/internal/TemplateEngine.java:
##########
@@ -1184,6 +1184,11 @@ private String escapeString(final CharSequence str) {
return StringParser.escapeString(str, (char) 0);
}
+ private static boolean isIgnorable(char c) {
+ return c == '\n' || c == '\r' || c == '\t' || c == '\f' || c == '\b';
+ }
+
+
Review Comment:
There's an extra blank line here. The codebase typically uses only one blank
line between methods and before Javadoc comments (see lines 758-759, 790-791,
1182-1183, 1186-1187). Consider removing the extra blank line for consistency.
```suggestion
```
##########
src/main/java/org/apache/commons/jexl3/internal/TemplateEngine.java:
##########
@@ -1086,54 +1086,54 @@ TemplateExpression parseExpression(final JexlInfo info,
final String expr, final
// nested immediate in deferred; need to balance count of '{'
& '}'
// closing '}'
switch (c) {
- case '"':
- case '\'':
- strb.append(c);
- column = StringParser.readString(strb, expr, column + 1,
c);
- continue;
- case '{':
- if (expr.charAt(column - 1) == immediateChar) {
- inner1 += 1;
- strb.deleteCharAt(strb.length() - 1);
- nested = true;
- } else {
- deferred1 += 1;
- strb.append(c);
- }
- continue;
- case '}':
- // balance nested immediate
- if (deferred1 > 0) {
- deferred1 -= 1;
+ case '"':
+ case '\'':
strb.append(c);
- } else if (inner1 > 0) {
- inner1 -= 1;
- } else {
- // materialize the nested/deferred expr
- final String src = escapeString(strb);
- final JexlInfo srcInfo = info.at(lineno, column);
- TemplateExpression dexpr;
- if (nested) {
- dexpr = new NestedExpression(
- escapeString(expr.substring(inested,
column + 1)),
+ column = StringParser.readString(strb, expr, column +
1, c);
+ continue;
+ case '{':
+ if (expr.charAt(column - 1) == immediateChar) {
+ inner1 += 1;
+ strb.deleteCharAt(strb.length() - 1);
+ nested = true;
+ } else {
+ deferred1 += 1;
+ strb.append(c);
+ }
+ continue;
+ case '}':
+ // balance nested immediate
+ if (deferred1 > 0) {
+ deferred1 -= 1;
+ strb.append(c);
+ } else if (inner1 > 0) {
+ inner1 -= 1;
+ } else if (!isIgnorable(c)) {
Review Comment:
The condition `!isIgnorable(c)` is always true at this point because we're
already in the `case '}':` branch, and `}` is never an ignorable character
(it's not in the set '\n', '\r', '\t', '\f', '\b'). This makes the condition
pointless and the entire else-if branch will always execute when we reach it.
This appears to be a logic error. The condition should be removed so that
the expression materialization logic executes unconditionally after checking
the deferred1 and inner1 counters.
```suggestion
} else {
```
##########
src/test/java/org/apache/commons/jexl3/Issues400Test.java:
##########
@@ -798,6 +799,38 @@ void testIssue441() {
Assertions.assertEquals(ctl, o);
}
+ @Test
+ void testIssue455a() {
+ final JexlEngine jexl = new JexlBuilder().create();
+ String code = "name -> `${name +\n name}`";
+ JexlScript script = jexl.createScript(code);
+ Object o = script.execute(null, "Hello");
+ String ctl = "HelloHello";
+ Assertions.assertEquals(ctl, o);
+ }
Review Comment:
The `isIgnorable` method includes `\r` (carriage return), `\f` (form feed),
and `\b` (backspace) as ignorable characters, but the test cases only verify
behavior with `\n` (newline) and `\t` (tab). Consider adding test cases that
verify these other ignorable characters are also correctly handled to ensure
comprehensive test coverage.
--
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]
