SEZ9 commented on PR #11496:
URL: https://github.com/apache/seatunnel/pull/11496#issuecomment-5707679973
Thanks for the follow-up commits (`0318a671b4`, `46e3d2cb35`, `00d5018647`).
Going through my earlier points:
**F1 (duplicate `-i` keys):** Throwing `ConfigCheckException` on duplicates
and documenting the behaviour change in `docs/en` and `docs/zh` is a reasonable
approach. Could you confirm the null-parsed-value path in
`backfillUserVariables` can no longer NPE? A small test for an empty `-i` value
would settle it.
**F3 (`[` / `]` in default values):** The depth-aware scan in
`PlaceholderUtils` looks like it should cover this, but the new
`testMultiplePlaceholdersWithDefault` test targets a different case. Could you
add a unit test with an array-literal default such as `${key:[a,b]}` so this is
explicitly covered?
**F2 (`withFallback(cleanSourceConfig)` leaking `-i` values into the final
config):** Was this changed? If the fallback merge stays, please explain why
exposing those values as top-level keys is acceptable, since secrets passed via
`-i` would appear in the rendered config.
**F4 / F5 (`System.setProperty` no longer called for `-i` variables):** If
dropping the JVM property export is intentional, please say so explicitly and
add a short note to the docs page that received the duplicate-key note; if not,
please restore it.
**F6 (quote heuristic stuck `insideQuotes`) and F8 (silent swallowing on
unbalanced `{`/`[` or unterminated quote):** Were these touched in
`ParameterSplitter`? F8 should fail fast with a clear error rather than
silently merging the rest of the input into the last token; for F6, a test with
a closing quote followed by a non-delimiter character would show whether it is
still an issue.
**F7 (tokenization change for existing `-i` values with braces/interior
quotes):** Does the docs update also cover the splitter behaviour change? If
not, one sentence there would be enough.
Separately, the latest review on this PR reports new issues introduced by
`0318a671b4` (including the `processVariable` changed-value check comparing a
`String` to a non-`String` `Object`), so those will need attention too. Once
the points above are addressed or argued and the tests are in, I'm happy to
take another pass.
<!-- streview-comment:1110 -->
--
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]
