mattcasters commented on code in PR #8745:
URL: https://github.com/apache/hop/pull/8745#discussion_r4172754074
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/LintResultGrouping.java:
##########
@@ -101,6 +103,56 @@ public static Map<LintFileCategory, Map<String,
List<LintResult>>> byCategoryAnd
return grouped;
}
+ /** What the results view sorts the findings of a severity on. */
+ public enum SortKey {
+ FILE,
+ RULE,
+ MESSAGE
+ }
+
+ /**
+ * The order of the findings within a severity: by the chosen column, then
by the others, so the
+ * same results always come out in the same order. Findings used to appear
in the order they were
+ * found, which moved a file's findings to the bottom each time it was
linted again.
+ */
+ public static Comparator<LintResult> order(SortKey key, boolean ascending) {
+ Comparator<LintResult> byFile =
+ Comparator.comparing((LintResult r) -> fileName(r),
String.CASE_INSENSITIVE_ORDER)
+ .thenComparing(r -> text(r.getFileName()),
String.CASE_INSENSITIVE_ORDER);
+ Comparator<LintResult> byRule =
+ Comparator.comparing((LintResult r) -> text(r.getRuleId()),
String.CASE_INSENSITIVE_ORDER);
+ Comparator<LintResult> byMessage =
+ Comparator.comparing((LintResult r) -> text(r.getMessage()),
String.CASE_INSENSITIVE_ORDER);
+
+ Comparator<LintResult> order =
+ switch (key) {
+ case RULE -> byRule.thenComparing(byFile).thenComparing(byMessage);
+ case MESSAGE ->
byMessage.thenComparing(byFile).thenComparing(byRule);
+ default -> byFile.thenComparing(byRule).thenComparing(byMessage);
+ };
+ return ascending ? order : order.reversed();
+ }
+
+ /** ERROR, WARNING and INFO in that order, whatever came first; anything
else after them. */
+ public static Comparator<String> severityOrder() {
+ List<String> known = List.of("ERROR", "WARNING", "INFO");
+ return Comparator.comparing(
+ (String severity) -> {
+ int index = known.indexOf(severity == null ? "" :
severity.toUpperCase());
+ return index < 0 ? known.size() : index;
+ })
+ .thenComparing(severity -> text(severity),
String.CASE_INSENSITIVE_ORDER);
+ }
+
+ private static String fileName(LintResult result) {
+ String path = text(result.getFileName());
+ return path.isEmpty() ? path : new
File(LintPathUtils.normalizePath(path)).getName();
Review Comment:
**[suggestion]** File order calls `LintPathUtils.normalizePath` from the
comparator, and that resolves and closes a VFS file object under the
synchronized `HopVfs.getFileObject` lock. `refreshResults` sorts on the UI
thread on every update and every column click, so a project-sized list pays
that cost O(n log n) times. A resolve that sometimes falls back to the raw path
can also make the comparator inconsistent.
**Suggestion:** Normalize each path once before sorting, or compare a
precomputed basename (`LintEditorGraphHelper.displayName` is the string the
File column already shows). Do not open a file object inside
`Comparator.compare`.
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/LinterConfigPlugin.java:
##########
@@ -116,15 +116,29 @@ public void widgetsCreated(GuiCompositeWidgets
compositeWidgets) {
@Override
public void widgetsPopulated(GuiCompositeWidgets compositeWidgets) {
- // Nothing to do.
+ enableDependentWidgets(compositeWidgets);
Review Comment:
**[bug]** `enableDependentWidgets` is only reached from `widgetsPopulated`
and `widgetModified`, but the configuration page never calls
`widgetsPopulated`. `ConfigPluginOptionsTab.showConfigPluginSettings` fills the
widgets and then calls `setWidgetsListener`, which only stores the listener.
`Button.setSelection` during that fill does not fire `SWT.Selection`, so this
does not run until the user edits something. Until then "Block Commits on
Warnings" stays enabled while "Block Git Commits on Lint Failures" is off. The
first click both stores the new value and then disables the box, so it cannot
be turned back off without enabling commit blocking. `LintResultsViewTest`
calls `enableDependentWidgets` directly, so it stays green.
**Suggestion:** After `setWidgetsListener` in
`ConfigPluginOptionsTab.showConfigPluginSettings`, call `widgetsPopulated` on
the listener. That is the only hook this screen will run before a click.
--
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]
