-
Notifications
You must be signed in to change notification settings - Fork 115
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
Empty value handling - rebase from #541 #598
Conversation
I will update the tests and accommodate the comments in my next commit. |
@radcortez @jbee please review |
07918f6
to
d254277
Compare
…tion Signed-off-by: Emily Jiang <emijiang6@googlemail.com>
e76f97d
to
99d03be
Compare
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.
Dropped some thoughts. The only crucial one to me is the question of how converters behave when they are given null
or empty/blank string values.
I'd think
- either a not present source lookup is never passed to the converter level and doing so manually is considered undefined behaviour to lift the burden of implementing these corner cases
- or any result from source lookup is passed to the converters but any empty value (not present) results in the same behaviour as the
Config
API would show, throwing an exception of some type.
api/src/main/java/org/eclipse/microprofile/config/spi/Converter.java
Outdated
Show resolved
Hide resolved
...n/java/org/eclipse/microprofile/config/tck/EmptyValue/EmptyValuesTestProgrammaticLookup.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/eclipse/microprofile/config/spi/Converter.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/eclipse/microprofile/config/spi/Converter.java
Outdated
Show resolved
Hide resolved
@Emily-Jiang I believe you have not yet pushed what you commented as done |
Signed-off-by: Emily Jiang <emijiang6@googlemail.com>
Signed-off-by: Emily Jiang <emijiang6@googlemail.com>
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.
I think everything is fine. Just confused about the empty test()
method.
tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyValue/EmptyValuesTest.java
Outdated
Show resolved
Hide resolved
@radcortez I think I addressed all of your comments as well. Please take another look at it and I would like to merge in tomorrow. |
tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyValue/EmptyValuesTest.java
Outdated
Show resolved
Hide resolved
...n/java/org/eclipse/microprofile/config/tck/EmptyValue/EmptyValuesTestProgrammaticLookup.java
Outdated
Show resolved
Hide resolved
...n/java/org/eclipse/microprofile/config/tck/EmptyValue/EmptyValuesTestProgrammaticLookup.java
Outdated
Show resolved
Hide resolved
tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyValue/EmptyValuesTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Emily Jiang <emijiang6@googlemail.com>
Signed-off-by: Emily Jiang <emijiang6@googlemail.com>
...n/java/org/eclipse/microprofile/config/tck/emptyvalue/EmptyValuesTestProgrammaticLookup.java
Outdated
Show resolved
Hide resolved
...n/java/org/eclipse/microprofile/config/tck/emptyvalue/EmptyValuesTestProgrammaticLookup.java
Outdated
Show resolved
Hide resolved
… into dmlloyd-empty-values
Signed-off-by: Emily Jiang emijiang6@googlemail.com
Fixes #446, Fixes #531, Fixes #532, Fixes #397.