mattcasters commented on code in PR #8557:
URL: https://github.com/apache/hop/pull/8557#discussion_r4083671736
##########
core/src/main/java/org/apache/hop/core/row/value/ValueMetaBase.java:
##########
@@ -1720,6 +1745,32 @@ public static Boolean convertStringToBoolean(String
string) {
|| "1".equals(string);
}
+ /**
+ * Converts a String to a Boolean, first matching the true and false text of
a Boolean conversion
+ * mask (ignoring case). Text that matches neither falls back to {@link
+ * #convertStringToBoolean(String)}.
+ *
+ * @param string the string to convert
+ * @return the Boolean, or null for an empty string
+ */
+ protected Boolean convertMaskedStringToBoolean(String string) {
+ if (Utils.isEmpty(string)) {
+ return null;
+ }
+ int slash = getBooleanMaskSeparator(conversionMask);
Review Comment:
**[bug]** Parsing a Boolean through conversion metadata ignores the mask, so
custom formats entered in the dialog this PR updates are stored as the wrong
value.
`convertMaskedStringToBoolean` only reads `this.conversionMask`.
`convertDataUsingConversionMetaData` for type Boolean calls `getBoolean` on the
String holder. Dates avoid this by resolving the pattern from
`conversionMetadata` inside `getDateFormat()`; Booleans do not.
`EnterValueDialog.getValue()` is that path: it puts the mask on the Boolean
value meta, sets it as the String meta's conversion metadata, and converts the
typed text. The new presets still work only because of the `Y`/`yes`/`true`/`1`
fallback, so `Ja` with mask `Ja/Nee` becomes `false` and the next `getString`
shows `Nee`. `T/F` and `on/off` fail the same way.
`Condition.createValueData()` uses the same conversion, and sorting a
`TableView` whose column value meta is a masked Boolean rewrites the cell
through it.
**Suggestion:** If this meta's mask is not a Boolean mask and
`conversionMetadata` has one, parse with
`conversionMetadata.getConversionMask()`. Add a test that sets the mask only on
a Boolean meta, attaches it with
`stringMeta.setConversionMetadata(booleanMeta)`, and checks
`convertDataUsingConversionMetaData("Ja")` and `("Nee")`.
##########
core/src/main/java/org/apache/hop/core/row/value/ValueMetaBase.java:
##########
@@ -1720,6 +1745,32 @@ public static Boolean convertStringToBoolean(String
string) {
|| "1".equals(string);
}
+ /**
+ * Converts a String to a Boolean, first matching the true and false text of
a Boolean conversion
+ * mask (ignoring case). Text that matches neither falls back to {@link
+ * #convertStringToBoolean(String)}.
+ *
+ * @param string the string to convert
+ * @return the Boolean, or null for an empty string
+ */
+ protected Boolean convertMaskedStringToBoolean(String string) {
+ if (Utils.isEmpty(string)) {
+ return null;
+ }
+ int slash = getBooleanMaskSeparator(conversionMask);
+ if (slash > 0) {
+ if (string.length() == slash && conversionMask.regionMatches(true, 0,
string, 0, slash)) {
Review Comment:
**[suggestion]** The true and false tokens are not trimmed, and a case-only
difference makes the false token unreachable.
`Ja / Nein` passes `getBooleanMaskSeparator` (the slash is not at either
end), but the tokens keep the surrounding spaces. Output therefore writes `Ja `
and ` Nein`. A reader that trims, including Text File Input trim "both", fails
the length check. Words outside the standard true list then become false, so
`Ja` does not round-trip. Separately, the true token is tested first with a
case-insensitive `regionMatches`, so mask `Yes/yes` writes both spellings and
reads both as true.
The new manual says to pick two different words and that reading ignores
case, but not that the words must differ ignoring case or that spaces around
the slash are part of the token.
**Suggestion:** Trim each side before comparing and before writing in
`convertBooleanToString`. If the trimmed sides are equal ignoring case, do not
treat the mask as a Boolean mask (or match the false token when the text equals
it). Cover `Ja / Nein` and `Yes/yes` in `ValueMetaBaseTest`.
##########
core/src/main/java/org/apache/hop/core/row/value/ValueMetaBase.java:
##########
@@ -1698,10 +1698,35 @@ protected synchronized BigDecimal
convertStringToBigNumber(String string)
// BOOLEAN + STRING
+ /**
+ * A Boolean format mask holds the text for true and the text for false,
separated by a single
+ * slash, for example {@code true/false}, {@code Y/N} or {@code 1/0}. A mask
with no slash, or
+ * with more than one (a date mask like {@code yyyy/MM/dd}), is not a
Boolean mask.
+ *
+ * @param mask the conversion mask
+ * @return the position of the separating slash, or -1 when the mask is not
a Boolean mask
+ */
+ static int getBooleanMaskSeparator(String mask) {
+ if (mask == null) {
+ return -1;
+ }
+ int slash = mask.indexOf('/');
+ if (slash <= 0 || slash == mask.length() - 1 || mask.indexOf('/', slash +
1) >= 0) {
+ return -1;
+ }
+ return slash;
+ }
+
protected String convertBooleanToString(Boolean bool) {
if (bool == null) {
return null;
}
+ int slash = getBooleanMaskSeparator(conversionMask);
Review Comment:
**[bug]** A Boolean conversion mask is not applied when the value is still
stored as a binary string.
`TextFileOutput.formatField` writes non-string fields with
`getBinaryString`. That method returns the original `byte[]` unchanged when
`isStorageBinaryString()` and `identicalFormat` are both true.
`setConversionMask` does call `compareStorageAndActualFormat()`, but that
method only updates `identicalFormat` for dates and numbers. For a Boolean the
flag stays at its initial `true`, so changing the mask to `Ja/Nee` never
reaches `convertBooleanToString`.
CSV Input with lazy conversion stores Boolean fields as the raw file bytes
(storage metadata is the string clone). Text File Output then writes those
bytes as-is, and the format column does nothing. Eager conversion is fine,
which is all `0118-boolean-format-text-file-output` exercises. Write to log and
Concat Fields call `getString` and are not affected.
**Suggestion:** In `compareStorageAndActualFormat()`, treat Booleans like
dates: `identicalFormat` is true only when the conversion mask is unchanged.
When both masks are empty, also require the lengths to match, because length
still selects `Y/N` versus `true`/`false`. Add a unit test where a
binary-string Boolean with storage mask `Y/N` returns the bytes of `Ja` from
`getBinaryString(true)` after `setConversionMask("Ja/Nee")`.
--
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]
