Skip to content

Added object that has integer value to float and double converter.#65

Merged
oza merged 1 commit intomsgpack:masterfrom
redboltz:add_integer_to_float_conv
Jan 21, 2014
Merged

Added object that has integer value to float and double converter.#65
oza merged 1 commit intomsgpack:masterfrom
redboltz:add_integer_to_float_conv

Conversation

@redboltz
Copy link
Copy Markdown

Added the conversion from msgpack's int format family to Java's float and double.
See
msgpack/msgpack#163

oza added a commit that referenced this pull request Jan 21, 2014
Added object that has integer value to float and double converter.
@oza oza merged commit 906dab1 into msgpack:master Jan 21, 2014
@oza
Copy link
Copy Markdown
Member

oza commented Jan 21, 2014

Merged. Thank you for your pull request, @redboltz!

@frsyuki
Copy link
Copy Markdown
Member

frsyuki commented Jan 21, 2014

This change is not consistent with MessagePackUnpacker#readDouble() and #readFloat(). One idea to make them consistent is that we can consider Float and Integer types (types in MessagePack) as subclasses of Number type.

@oza
Copy link
Copy Markdown
Member

oza commented Jan 21, 2014

Oops, I overlooked the problem. Thank you for pointing, @frsyuki.

@redboltz:
As @frsyuki mentioned, we should keeping consistency of return value not only between Converter#readDouble() and MessagePackUnpacker#readDouble() but also between Converter#readFloat() and MessagePackUnpacker#readFroat(). To keep the consisntency, @frsyuki's idea looks good to me. Could you resend your pull request again after adding readNumber() API to keep consistency?

@oza
Copy link
Copy Markdown
Member

oza commented Jan 21, 2014

I'll revert your commit once.

@frsyuki
Copy link
Copy Markdown
Member

frsyuki commented Jan 21, 2014

@oza It sounds good idea to add readNumber() method to Unpacker interface. Thus MessagePack can keep strong-typing.
On the other hand, msgpack-java users need to use Number class to receive data from JavaScript. They can't use int type.
@redboltz To solve this problem, an idea is to add an option like "allowDowncastNumberToInteger" to template generator, or MessagePackUnpacker and Converter. Then when users can reasonably decide to enable an option at Java-side when they need to choose whether they change JavaScript-side implementation to strictly distinguish integers from floats, or change Java-side implementation to loosely accept both integers and floats.

@redboltz
Copy link
Copy Markdown
Author

I'm trying to implement readNumber() approach. I met a problem. It seems that java.lang.Number doesn't have bigIntegerValue(). java.lang.Long doen't cover msgpack's uint64.

I created a class named NumberAccept and that has "value" field. The type of "value" filed is Number. I tried to use NumberValue instead of Number because NumberValue has the method bigIntegerValue(). But NumberValue and java.lang.Long, Integer, ... are sibiling classes. I think that it is a cross-cast problem.

Any ideas?

@frsyuki
Copy link
Copy Markdown
Member

frsyuki commented Jan 21, 2014

They can call intValue() or longValue() to convert them to int or long. It becomes users' work how to convert Number to integers.
2 ideas:

  1. add Number Unpacker#readNumber() which actually returns NumberValue.
    • This approach requires users to write extra code to convert Number to desired type such as int, long, float or double.
    • But it's programmable how to convert them.
  2. add a configurable option to MessageUnpacker and Convert such as "allowIntegerToBeFloat" which changes behavior of readFloat() and readDouble() to accept integer values.
    • This approach doesn't require users to convert Number to desired types.
    • But it's not programmable how to convert them because we decide the behavior of the option.

@redboltz
Copy link
Copy Markdown
Author

@frsyuki , Thanks you for your advice. I've finished implementing readNumber() function. Could you check it?
#66

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.

3 participants