gnodet-bot commented on code in PR #27181:
URL: https://github.com/apache/camel/pull/27181#discussion_r4153034000
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/main/java/org/apache/camel/dsl/yaml/validator/YamlValidator.java:
##########
@@ -134,6 +134,27 @@ public List<Error> validate(String content) throws
Exception {
return validate(content, Set.of());
}
+ /**
+ * The 1-based line of the YAML content a validation error points at (the
line of the key of a property that is not
+ * allowed), or 0 when it has no location or the content does not parse.
+ */
+ /**
+ * A validation error as reports print it, with the line of the YAML
content it points at in front when that is
+ * known: "Line 12: /0/route/from/steps/0/log: property 'logLevel' is not
defined in the schema...".
+ */
+ public static String describe(String content, Error error) {
+ int line = content != null ? lineOf(content, error) : 0;
+ return line > 0 ? "Line " + line + ": " + error :
String.valueOf(error);
+ }
+
+ public static int lineOf(String content, Error error) {
+ if (error == null || error.getInstanceLocation() == null) {
+ return 0;
+ }
+ return YamlPointerLines.line(YamlPointerLines.root(content),
error.getInstanceLocation().toString(),
+ error.getMessage());
+ }
Review Comment:
💡 **Repeated YAML tree parsing.** `lineOf()` calls
`YamlPointerLines.root(content)` on every invocation, re-parsing the entire
YAML into a SnakeYAML node tree each time. Both callers (`ValidateMojo` and
`YamlValidateCommand`) loop over errors calling `describe()` per error — so for
a file with N schema errors, the tree is parsed N times.
`SourceValidator.formatSchemaErrors(errors, content)` in this same PR does
it correctly: it parses once before the loop and passes the root node into
`YamlPointerLines.line(root, ...)` for each error.
Not a production-critical issue (validation reports aren't hot paths), but
inconsistent with the approach 30 lines away. A `describeAll(String content,
List<Error> errors)` method (or documenting that `lineOf` is for single-error
use and callers with lists should use `YamlPointerLines` directly) would close
the gap.
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/main/java/org/apache/camel/dsl/yaml/validator/YamlValidator.java:
##########
@@ -134,6 +134,27 @@ public List<Error> validate(String content) throws
Exception {
return validate(content, Set.of());
}
+ /**
+ * The 1-based line of the YAML content a validation error points at (the
line of the key of a property that is not
+ * allowed), or 0 when it has no location or the content does not parse.
+ */
Review Comment:
💡 **Orphaned Javadoc.** Two consecutive `/** */` blocks are stacked above
`describe()`. The first one (lines 137–140) describes `lineOf()` but is
disconnected — it documents nothing. Move it onto `lineOf()` (which has no
Javadoc), or merge the two blocks into one on `describe()`.
```suggestion
/**
* A validation error as reports print it, with the line of the YAML
content it points at in front when that is
* known: "Line 12: /0/route/from/steps/0/log: property 'logLevel' is
not defined in the schema...".
*/
```
--
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]
