mattcasters commented on code in PR #8743:
URL: https://github.com/apache/hop/pull/8743#discussion_r4172771488
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java:
##########
@@ -548,11 +599,35 @@ private static Object extractFieldFromAction(ActionMeta
actionMeta, String field
}
}
- private static boolean hasDefaultGeneratedName(String name) {
+ /**
+ * Whether a transform or action still has the name Hop Gui gave it: the
plugin's name, such as
+ * "Table input", followed by a number when that name was taken, as in
"Dummy (do nothing) 2".
+ *
+ * <p>Only "Transform 1" and "Action 1" used to count, and Hop Gui never
generates those. The
Review Comment:
**[suggestion]** This method now flags the plugin name Hop Gui assigns, and
this comment correctly says Hop Gui never generates "Transform 1". NAMING-004
stays enabled, and its description in `hop-lint-core.yml` is still
"Auto-generated names such as 'Transform 1' make logs and error messages hard
to trace back to the pipeline". `generateErrorMessage` prints that description
plus `(current value: true)`, so a transform left named "Table input" is
reported with the obsolete example and not the name.
**Suggestion:** Change the NAMING-004 description to the plugin name and the
" <number>" suffix this method recognizes. Optionally include the transform or
action name in the finding instead of only the boolean.
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java:
##########
@@ -1019,6 +1094,11 @@ private static boolean evaluateCondition(
case NOT_EMPTY:
return Utils.isEmpty(fieldValue.toString());
+ case IS_EMPTY:
+ // The opposite of NOT_EMPTY, so that a clause can require that a
field is not set, as in
+ // "a plain http:// URL with no login": url NOT_MATCHES_PATTERN https,
httpLogin IS_EMPTY.
Review Comment:
**[nit]** This example does not match the return polarity or the manual.
`IS_EMPTY` is a violation when the value is present (`!Utils.isEmpty`), and
`NOT_MATCHES_PATTERN` against an https pattern is a violation when the URL is
https. Under `allOf` that reports an https URL whose login is set, not "a plain
http:// URL with no login". The adoc example is the one that matches the code:
`url` `MATCHES_PATTERN` `^https://.*` together with `httpLogin` `IS_EMPTY`
reports http with a login.
**Suggestion:** Replace this comment with that example so a later edit does
not invert the clauses.
##########
plugins/misc/lint/src/main/resources/hop-lint-core.yml:
##########
@@ -315,10 +316,11 @@ rules:
- TableInput
allOf:
- targetField: sql
- condition: MATCHES_PATTERN
+ condition: NOT_MATCHES_PATTERN
conditionValue: "(?is).*\\bselect\\s+\\*.*"
- targetField: rowLimit
- condition: NOT_EMPTY
+ condition: NOT_MATCHES_PATTERN
+ conditionValue: "^\\s*0*\\s*$"
Review Comment:
**[suggestion]** `^\s*0*\s*$` only treats a blank or all-zero string as no
limit. `evaluateCondition` returns false for `NOT_MATCHES_PATTERN` when the
value is null, so a Table Input whose `rowLimit` was never set is not reported.
That field defaults to null (`TableInputMeta` only assigns `"0"` in
`setDefault()`, which load does not call), and the runtime reads it as
unlimited: `Const.toIntExpanded(null, 0)` and `Database` calls `setMaxRows`
only when the limit is `> 0`. The limit widget also calls
`enableExpandedInteger()`, so `0k`, `+0`, `0.000`, and negatives are unlimited
too and also slip through. Leaving `${LIMIT}` unflagged is right; the new test
says a variable is intentional.
**Suggestion:** Treat null like `""` for this clause (fall through the null
short-circuit with an empty string for `NOT_MATCHES_PATTERN`, or compare with
`Const.toIntExpanded` and skip values that contain `${`). Cover null next to
the `""` and `"0"` cases.
--
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]
