Skip to content

Provide factory to obtain JsonValue from java.lang.Number #302

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

Merged
merged 2 commits into from
Sep 22, 2021

Conversation

jbescos
Copy link
Member

@jbescos jbescos commented Sep 20, 2021

Relates to #180

* @since 2.1
*/
public JsonNumber createValue(Number value) {
throw new UnsupportedOperationException();
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to have the default impl which would just call createValue(value.intValue()) for int, createValue(value.doubleValue()) for double etc? In any case, the parameter name should be renamed to sth more meaningful, what about number?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is more or less how I solved it in the implementation:
https://github.com/eclipse-ee4j/parsson/pull/30/files

From my point of view, it makes more sense to do it in the API if we are able to cover every scenario. Otherwise it will be half done.

There is one scenario where Number is not known, for example, if somebody implements its own Number. In this case I think we cannot reuse any existing method easily. What is your opinion?.

Regarding the parameter name, you mean to replace 'value' by 'number', right?.

Copy link
Contributor

Choose a reason for hiding this comment

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

if somebody implements its own Number

handle known/simple cases and throw UOE for unknowns? I agree it does not cover every scenario but even ~75% at this level is IMO good enough.

Regarding the parameter name, you mean to replace 'value' by 'number', right?.

yes

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated it with the supported types and the 'value' was changed to 'number'.

Copy link
Contributor

Choose a reason for hiding this comment

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

could the exception contain some message, please?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Jorge Bescos Gascon <[email protected]>
Copy link
Contributor

@lukasj lukasj left a comment

Choose a reason for hiding this comment

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

LGTM

@lukasj lukasj linked an issue Sep 22, 2021 that may be closed by this pull request
@lukasj lukasj merged commit e865594 into jakartaee:master Sep 22, 2021
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

Successfully merging this pull request may close these issues.

Provide factory to obtain JsonValue from java.lang.Number
2 participants