DanielLeens commented on PR #12810:
URL: https://github.com/apache/seatunnel/pull/12810#issuecomment-6430714059

   Thanks @NganWave, that is a very reasonable plan, and thank you for being so 
transparent about your time.
   
   Tracing the whole masking path (`ParameterSplitter` at the CLI, 
`ConfigBuilder`, `ConfigUtil`, down to `ReadonlyConfig`) in a dedicated PR is 
the right shape for that work, so I am fine with the broad audit living in a 
follow-up.
   
   One small, narrow suggestion that can stay in this PR: the two error 
messages this PR newly introduces in `PlaceholderUtils` (conflicting defaults 
and unterminated placeholder) currently echo the raw text. Those are new 
surfaces created by this change rather than pre-existing ones, so simply not 
including the raw default or input slice in those two messages would keep this 
PR from adding to what the follow-up has to clean up. Everything else on the 
masking side can wait for the follow-up.
   
   The other items from the earlier review (including the failing 
`ConfigShadeTest.testPlaceholders`) still apply whenever you pick this back up. 
No rush at all on a draft.


-- 
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]

Reply via email to