bamaer commented on code in PR #8877:
URL: https://github.com/apache/hop/pull/8877#discussion_r4250909133
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectLintYamlExporter.java:
##########
@@ -98,9 +124,11 @@ private static boolean differsFromPackDefault(
return desired.isEnabled() != packDefault.isEnabled()
|| !Objects.equals(desired.getSeverity(), packDefault.getSeverity())
|| !Objects.equals(desired.getAdditionalParameters(),
packDefault.getAdditionalParameters())
- || !Objects.equals(desired.getConditionValue(),
packDefault.getConditionValue())
+ || !sameText(desired.getConditionValue(),
packDefault.getConditionValue())
Review Comment:
Fixed. `clauseShape` now includes the values of every clause after the
first, so changing one makes the rule a redefinition: the Rules Manager asks
first and the whole rule is written with every clause. A first-clause value
change stays an override. Tests: `aChangedValueInALaterClauseRedefinesTheRule`
changes SQL-002's row-limit pattern and reads the file back with both patterns,
and `aChangedValueInTheFirstClauseStaysAnOverride`. The docs page on rule packs
says the same.
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/RuleBuilderDialog.java:
##########
@@ -542,15 +594,94 @@ private RuleCondition selectedCondition() {
private void updateValueField() {
RuleCondition condition = selectedCondition();
- if (condition != null) {
- valueLabel.setVisible(condition.requiresValue());
- valueText.setVisible(condition.requiresValue());
+ boolean usesValue = condition == null || condition.requiresValue();
+ valueLabel.setVisible(usesValue);
+ valueText.setVisible(usesValue);
+ if (condition != null && condition.requiresValue()) {
+ valueText.setToolTipText(condition.getDescription());
+ }
+ shell.layout(true, true);
+ }
- if (condition.requiresValue()) {
- valueText.setToolTipText(condition.getDescription());
+ /**
+ * A field the new target also has stays, with its condition and value; a
field it does not have
+ * empties its clause. Back on the rule's own target, an emptied clause gets
the rule's clause
+ * back, so a trip to another target does not lose a field the list does not
know, such as a Table
+ * Input's {@code sql}.
+ */
+ private void targetChanged() {
+ int targetIndex = targetCombo.getSelectionIndex();
+ if (targetIndex < 0 || targetIndex == shownTarget) {
+ return;
+ }
+ shownTarget = targetIndex;
+ RuleTarget target = RuleTarget.values()[targetIndex];
+ List<String> fields = RuleTargetFields.getFieldsForTarget(target);
+ List<RuleClause> original = target == rule.getTarget() ? rule.getClauses()
: List.of();
Review Comment:
Fixed. A target change that empties a clause now keeps a copy of that
editing clause and its target. Going back to that target restores the copy
while the clause is still empty, and `rule.getClauses()` is no longer used.
Test: `backOnItsOwnTargetAClauseKeepsItsEdits`.
##########
plugins/misc/lint/src/test/java/org/apache/hop/lint/RuleBuilderValidationTest.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hop.lint;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import java.util.Set;
+import org.junit.jupiter.api.Test;
+
+/** The checks the rule editor runs on OK, before anything is written to
hop-lint.yml. */
+class RuleBuilderValidationTest {
+
+ /**
+ * An empty extra clause passed the editor, which only checked the clause on
show, and the save
+ * then failed with a NullPointerException message.
+ */
+ @Test
+ void anEmptyClauseIsIncomplete() {
Review Comment:
Added SWTBot tests that drive `validate()` through OK, with errors collected
through a package-private `errorReporter` because a native message box can't be
driven from a test: `anEmptyClauseThatIsNotOnShowIsRefused` (also asserts the
empty row is selected), `aMissingValueInAClauseThatIsNotOnShowIsRefused` and
`aLeftoverValueOnAConditionWithoutValueIsDropped`. Checking only the selected
row makes the first two fail.
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/registry/ProjectYamlOverlay.java:
##########
@@ -112,9 +121,18 @@ public static ProjectRuleOverlay fromMap(
Object value = ruleData.get("helpUri");
helpUri = value != null ? value.toString().trim() : "";
}
+ // Renaming a pack rule used to write it out in full, which replaced the
pack rule for good.
Review Comment:
Done. The history is gone from this comment and from the others like it in
the PR (the clause loop, `putRule`, the Rules Manager and the exporter); where
a comment is still needed it states the invariant.
--
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]
