-
Notifications
You must be signed in to change notification settings - Fork 14.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MINOR: leverage preProcessParsedConfig within AbstractConfig #19259
Conversation
This is a kind of big fix, so please add a test |
Done. Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR, left a comment
props.put("foo.bar", "abc"); | ||
props.put("setting", "def"); | ||
TestConfig config = new TestConfig(props); | ||
assertEquals("success", config.get("preprocess")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also check foo.bar
and setting
are not in the map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If foo.bar
and setting
is not defined in CONDEF
, it will be removed so I deleted these entry.
Thanks for your review and suggestion !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
In past, we have
AbstractConfig#preProcessParsedConfig
but did not use its return valueThis PR fix such issue.
Reviewers: Ken Huang [email protected], Chia-Ping Tsai [email protected]