-
Notifications
You must be signed in to change notification settings - Fork 144
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
[Followup] Use asList method in some existing configOptions #18
Conversation
.stringType() | ||
.asList() | ||
.noDefaultValue() | ||
.withDescription("List config key5"); |
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 we don't set this option and call method conf.get(emptyListStringOption) directly, what will the result be?
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.
It will return null.
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.
Is the same behavior as Flink?
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.
Yes. I have checked.
Codecov Report
@@ Coverage Diff @@
## master #18 +/- ##
============================================
- Coverage 56.27% 55.43% -0.84%
+ Complexity 1168 1095 -73
============================================
Files 152 143 -9
Lines 8397 8034 -363
Branches 812 784 -28
============================================
- Hits 4725 4454 -271
+ Misses 3408 3324 -84
+ Partials 264 256 -8
Continue to review full report at Codecov.
|
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
### What changes were proposed in this pull request? Remove redundant package. Introduced in #18. ### Why are the changes needed? better code ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? No
What changes were proposed in this pull request?
Use asList method in some existing configOptions
Why are the changes needed?
Directly use the asList method in ConfigOptions to get the config list values, and then avoid splitting values by users.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
UTs.