mattcasters commented on code in PR #8824:
URL: https://github.com/apache/hop/pull/8824#discussion_r4231998090
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleBuilderDialog.java:
##########
@@ -88,12 +97,16 @@ public CustomLintRule open() {
populateControls();
shell.pack();
- // Roomy enough for the clause table and its buttons on top of the fields
that were here
- // before; the dialog is resizable if a rule grows more clauses than that.
- shell.setSize(700, 660);
+ // Roomy enough for the clause table, the fields and the tags table, but
never taller than the
+ // screen: on a laptop display an 820 pixel dialog put OK and Cancel out
of reach. Below the
+ // minimum the fixed rows would run into the button bar, so the dialog
does not shrink past it.
+ Rectangle screen = parent.getMonitor().getClientArea();
+ int height = Math.min(820, screen.height);
+ shell.setMinimumSize(560, Math.min(700, height));
+ shell.setSize(700, height);
shell.setLocation(
- parent.getLocation().x + (parent.getSize().x - 700) / 2,
- parent.getLocation().y + (parent.getSize().y - 660) / 2);
+ Math.max(screen.x, parent.getLocation().x + (parent.getSize().x - 700)
/ 2),
+ Math.max(screen.y, parent.getLocation().y + (parent.getSize().y -
height) / 2));
Review Comment:
**[bug]** The new height is the monitor work area (capped at 820), but only
the top edge is clamped. `Shell.setSize` includes trim, and the y coordinate is
still the old center-on-parent formula. When the Hop window sits in the lower
part of the screen, the dialog extends past the work area and OK/Cancel — the
buttons this change was meant to keep reachable — open off-screen. The dialog
is application-modal, so the only recovery is dragging the title bar.
**Suggestion:** Clamp both origin and size so the outer bounds stay inside
the monitor work area:
```java
int width = Math.min(700, screen.width);
int height = Math.min(820, screen.height);
int x = parent.getLocation().x + (parent.getSize().x - width) / 2;
int y = parent.getLocation().y + (parent.getSize().y - height) / 2;
x = Math.max(screen.x, Math.min(x, screen.x + screen.width - width));
y = Math.max(screen.y, Math.min(y, screen.y + screen.height - height));
shell.setMinimumSize(Math.min(560, width), Math.min(700, height));
shell.setSize(width, height);
shell.setLocation(x, y);
```
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/YamlRulePackParser.java:
##########
@@ -481,9 +523,120 @@ public static CustomLintRule parseCustomRule(
if (parameters != null && !parameters.isEmpty()) {
customRule.setAdditionalParameters(new HashMap<>(parameters));
}
+ applyDocumentation(customRule, ruleData);
+ warnUnknownKeys(ruleId, packId, ruleData, CUSTOM_RULE_KEYS);
return customRule;
}
+ /** Read the keys that describe a rule without changing what it checks: tags
and helpUri. */
+ private static void applyDocumentation(CustomLintRule rule, Map<String,
Object> ruleData) {
+ String helpUri = stringValue(ruleData.get("helpUri"), null);
+ if (!Utils.isEmpty(helpUri)) {
+ rule.setHelpUri(helpUri.trim());
+ }
+ rule.setTags(tagsValue(ruleData.get("tags"), rule.getId(),
rule.getPackId()));
+ }
+
+ /**
+ * Read a {@code tags:} block. Each value is a string or a list of strings.
+ *
+ * <p>Anything else is logged and left out rather than failing the pack:
tags never change what a
+ * rule finds, so a malformed one is no reason to stop linting.
+ */
+ static Map<String, List<String>> tagsValue(Object value, String ruleId,
String packId) {
+ return tagsValue(value, ruleId, packId, false);
+ }
+
+ /**
+ * @param keepEmpty whether a key with no values is kept: in a project
override it removes the
+ * pack's tag of that key, on a rule it means nothing
+ */
+ static Map<String, List<String>> tagsValue(
+ Object value, String ruleId, String packId, boolean keepEmpty) {
+ Map<String, List<String>> tags = new LinkedHashMap<>();
+ if (value == null) {
+ return tags;
+ }
+ if (!(value instanceof Map<?, ?> map)) {
+ warn(
+ "Warning: tags of rule '"
+ + ruleId
+ + "' "
+ + where(packId)
+ + " are not a mapping and are ignored.");
+ return tags;
+ }
+ for (Map.Entry<?, ?> entry : map.entrySet()) {
+ String key = entry.getKey() != null ? entry.getKey().toString().trim() :
"";
+ Object tagValue = entry.getValue();
+ if (key.isEmpty() || tagValue instanceof Map) {
+ warn(
+ "Warning: tag '"
+ + key
+ + "' of rule '"
+ + ruleId
+ + "' "
+ + where(packId)
+ + " is ignored: a tag is a string or a list of strings.");
+ continue;
+ }
+ // A value given twice is kept once: SARIF requires the tags of a rule
to be unique.
+ List<String> values = new ArrayList<>(new
LinkedHashSet<>(stringListValue(tagValue)));
+ if (keepEmpty || !values.isEmpty()) {
+ tags.put(key, values);
+ }
+ }
+ return tags;
+ }
+
+ /**
+ * Log every key on a rule that the linter does not read.
+ *
+ * <p>An unknown key is dropped, and a typo such as {@code tag:} for {@code
tags:} or {@code
+ * severty:} would otherwise change the rule with nothing to show for it.
+ */
+ private static void warnUnknownKeys(
+ String ruleId, String packId, Map<String, Object> ruleData, Set<String>
knownKeys) {
+ unknownKeyWarnings(ruleId, packId, ruleData,
knownKeys).forEach(YamlRulePackParser::warn);
+ }
+
+ /** One warning for each key on the rule that is not one of the known keys.
*/
+ static List<String> unknownKeyWarnings(
+ String ruleId, String packId, Map<String, Object> ruleData, Set<String>
knownKeys) {
+ List<String> warnings = new ArrayList<>();
+ if (ruleData == null) {
+ return warnings;
+ }
+ for (Object key : ruleData.keySet()) {
+ String name = String.valueOf(key);
+ if (knownKeys.contains(name)) {
+ continue;
+ }
+ StringBuilder warning =
+ new StringBuilder("Warning: rule '")
+ .append(ruleId)
+ .append("' ")
+ .append(where(packId))
+ .append(" has an unknown key '")
+ .append(name)
+ .append("', which is ignored.");
+ String closest = RuleRegistry.closestId(name, knownKeys);
+ if (closest != null) {
+ warning.append(" Did you mean ").append(closest).append("?");
+ }
+ warnings.add(warning.toString());
+ }
+ return warnings;
+ }
+
+ private static String where(String packId) {
+ return RulePackIds.PROJECT.equals(packId) ? "in hop-lint.yml" : "in pack
'" + packId + "'";
Review Comment:
**[suggestion]** An unknown key in a project override is reported only as
"in hop-lint.yml", and `warn` dedupes on that text alone
(`LintWarnings.logOnce(warning, warning)`). `RuleRegistry.unknownRuleWarning`
includes the file name and keys the once-log with `absolutePath + warning`. A
second project with the same typo, such as `severty` on `DB-001`, is then
logged only at detailed level, and even the first line does not say which file.
**Suggestion:** Pass the `hop-lint.yml` path into `where` for
`RulePackIds.PROJECT`, and call `LintWarnings.logOnce(path + '\n' + warning,
warning)` the same way the unknown-rule-id warning does. Pack warnings can keep
using the pack id.
--
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]
