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.

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

Clarification of config.getValue() for an Optional #675

Open
Joseph-Cass opened this issue Feb 11, 2021 · 2 comments
Open

Clarification of config.getValue() for an Optional #675

Joseph-Cass opened this issue Feb 11, 2021 · 2 comments
Milestone

Comments

@Joseph-Cass
Copy link

Description
Should config.getValue("int.missing.key", OptionalInt.class) throw a NoSuchElementException according to this bit of spec? (where int.missing.key is not defined in a config source)

* @throws java.util.NoSuchElementException
* if the property is not defined or is defined as an empty string or the converter returns {@code null}

If yes, It seems like this could be a relatively common mistake, which suggests to me it may be worth having a TCK for (If there isn't one already- I couldn't find one)

@Emily-Jiang
Copy link
Member

Emily-Jiang commented Feb 11, 2021

Good observation! This was an oversight when adding OptionalInt, OptionalDouble, OptionalLong. Effectively, when the type is Optionalxxx, the getValue will return an Optionalxxx. If we throw NoSuchElementException, this defeats the purpose of Optionalxxx. With this, I suggest to update javadoc to not to throw this exception if the class type is Optionalxxx. What others think?

@radcortez
Copy link
Contributor

A related case that we may also want to clarify is when you call getOptionalValue("property", Optional*.class). Right now in SR Config, we just return a Optional<Optional*>, which I think it is ok. Some may expect this to be flattened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants