Skip to content
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.

Sign up for GitHub

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

Jump to bottom

608 - nullValue clarification #618

Merged
merged 1 commit into from
Oct 26, 2020
Merged

608 - nullValue clarification #618

merged 1 commit into from
Oct 26, 2020

Conversation

Emily-Jiang
Copy link
Member

Signed-off-by: Emily Jiang emijiang6@googlemail.com

@Emily-Jiang
Copy link
Member Author

Thank you @Joseph-Cass @radcortez for reviewing this!
@jbee please take a look as well. This is what you voted for. Hopefully this meets your expectation!

jbee
jbee approved these changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,
just a grammar suggestion - then I am not a native speaker but that is how I think it should be.

spec/src/main/asciidoc/converters.asciidoc Outdated Show resolved Hide resolved
@Emily-Jiang Emily-Jiang force-pushed the nullValue-clarification branch 2 times, most recently from 2a0279a to 1c6700f Compare October 8, 2020 20:47
@Emily-Jiang
Copy link
Member Author

@radcortez @jbee please review

jbee
jbee suggested changes Oct 23, 2020
View reviewed changes
Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly phrasing.
If I don't misunderstand the test I think we should not write it in a way that expects a deployment exception.

api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java Outdated Show resolved Hide resolved
api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java Outdated Show resolved Hide resolved
api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java Outdated Show resolved Hide resolved
.../microprofile/config/tck/converters/convertToNull/ConvertedNullValueBrokenInjectionTest.java Show resolved Hide resolved
@Emily-Jiang
Clarify the behaviour when a converter returns null
e32cdf0 
Signed-off-by: Emily Jiang <emijiang6@googlemail.com>
@Emily-Jiang
Copy link
Member Author

@jbee @radcortez please approve if you have no other comments.

@Emily-Jiang Emily-Jiang merged commit bc0ea99 into eclipse:master Oct 26, 2020
@Joseph-Cass Joseph-Cass mentioned this pull request Oct 27, 2020
Closed
@Azquelt Azquelt mentioned this pull request Nov 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Joseph-Cass Joseph-Cass Joseph-Cass left review comments

@jbee jbee jbee approved these changes

@radcortez radcortez radcortez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Emily-Jiang @jbee @radcortez @Joseph-Cass