mattcasters commented on PR #8419:
URL: https://github.com/apache/hop/pull/8419#issuecomment-5711177038
Checked `4a10feb` ("Issue #8415 : Address review feedback on the Text
chunker transform") against the original review on `226db33`, not against the
recap.
### Fixed
1. **Error hop.** `TextChunkerMeta.supportsErrorHandling()` returns `true`,
so the `putError` branch is reachable.
2. **Combo wipe.** The focus listener that `removeAll()`'d every field combo
is gone. (The replacement has a leftover, below.)
3. **Variables.** `chunkSize` and `chunkOverlap` are `String`, resolved once
in `init()` into `TextChunkerData`. `check()` resolves before validating.
Covered by `resolvesChunkSizeAndOverlapFromVariables`.
4. **Overlap.** Next start is always `Math.max(start + 1, chunkEnd -
overlap)`. Boundaries use `Character.isWhitespace`. `testVeryLongWord` asserts
overlap with no whitespace.
5. **RDBMS JSON secrets.** `formatConnection` no longer dumps the raw node.
`prettyJson` runs `SecretRedaction`. Tests assert encrypted passwords,
`proxyPassword` and `bearerToken` do not appear.
6. **Hop XML secrets.** `serializeElement` redacts via `SecretRedaction`
rather than extra `SKIP_TAGS`. Bare `key` is left alone on purpose;
`lookupKeys: customer_id` survives next to `password: **redacted**`. IT
`0117-text-chunker-redaction` covers this.
7. **Paragraph packing.** Consecutive paragraphs pack up to `maxSize` by
taking the source slice between first and last, so start/end still address the
original text (including `\n \n` separators).
`packsConsecutiveParagraphsUpToMaxSize` covers the contract. IT
`0115-text-chunker-paragraph` uses `chunkSize=25` so the three short paragraphs
still split.
8. **Dialog.** Rewritten onto grouped `GuiCompositeWidgets` (`BOXES`). (The
rewrite has leftovers, below.)
9. **Zero chunk size.** `init()` fails when the resolved size is not
positive. Covered by `initFailsWhenTheResolvedChunkSizeIsNotPositive`.
10. **Tooltip.** `article/blog=Markdown`.
11. **Empty text log.** Message says an empty chunk row is emitted.
Packing via `text.substring(first.start(), last.end())` is the right
alternative to joining with `\n\n`.
### Leftovers (dialog rewrite on `4a10feb`)
These are new, not residual copies of the original bugs.
1. **Changing chunking strategy in the dialog is silently ignored.**
`GuiCompositeWidgets` fills enum combos with `enum.toString()` and reads them
back with `Enum.valueOf`. `ChunkingStrategyType.toString()` returns
`"Character"` / `"Paragraph"` / `"Structure"`, not `CHARACTER`.
`Enum.valueOf(..., "Paragraph")` throws, the catch keeps the previous value,
and OK does not persist a strategy change. `ContentType` does not override
`toString()`, so that combo is fine. `PluginCatalogMeta`'s `DetailLevel` is the
working pattern: no custom `toString()`. The old dialog used `.name()` as
items, which did round-trip.
2. **Stream-field dropdowns are not filled.** `GuiCompositeWidgets` builds
an SWT `Combo` when `variables` is off. `TextChunkerDialog.setComboItems` only
handles `ComboVar` and `CCombo`, so it is a no-op. Saved names still appear
(set before this method runs); the list of previous-transform fields does not.
In-tree pattern is `DdlDialog`: `widgets.setComboValues(widgetId, fieldNames)`.
3. **Content-type enablement does not follow a live strategy change.**
`comboText()` has the same `ComboVar`/`CCombo` gap, so it never reads the
strategy widget. `enableFields()` also compares `STRUCTURE.name()`
(`"STRUCTURE"`) to the displayed `toString()` (`"Structure"`). Initial open
uses the meta fallback and looks right; switching Character → Structure in the
dialog does not enable the content-type widgets.
Small nits: leftover javadoc in `CharacterChunkingStrategy` ("Characters
that are considered word separators.") sitting on `chunk()`; `TextChunkerData`
has two stacked javadocs on `chunkSize`, the first of which still describes
`inputFieldIndex`.
--
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]
