Skip to content

#259: Rename data access methods#265

Merged
xerial merged 5 commits intomsgpack:v07-developfrom
xerial:rename-dataaccess
Jul 4, 2015
Merged

#259: Rename data access methods#265
xerial merged 5 commits intomsgpack:v07-developfrom
xerial:rename-dataaccess

Conversation

@xerial
Copy link
Copy Markdown
Member

@xerial xerial commented Jun 30, 2015

No description provided.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm surprised this works. I would have thought that the toJson would have quoted the string (as ImmutableRawValueImpl does). You actually get a Variable$StringValueAccessor which does't quote the string.

This depends on the output format of toJson however you wrote in the java doc

Do not write code that depends on the resulting json format.

but this depends directly on the output. Should this be using toJson or a method that would be more predictable?

Value#toJson() is expected to return a valid json since its name. Some
valid MessagePack values are invalid in JSON such as ExtensionValue,
non-string keys in a MapValue, NaN or Infinite.
@frsyuki
Copy link
Copy Markdown
Member

frsyuki commented Jul 1, 2015

@xerial I created this pull-request: xerial#2

@xerial
Copy link
Copy Markdown
Member Author

xerial commented Jul 1, 2015

@frsyuki
Thanks for the fix to produce json string in all of toJson() methods.

@xerial
Copy link
Copy Markdown
Member Author

xerial commented Jul 1, 2015

@komamitsu
Could you help me to fix the failed tests?

Thanks.

@komamitsu
Copy link
Copy Markdown
Member

@xerial Created xerial#3

Use RawValue#toString instead of Value#toJson in MessagePackParser
xerial added a commit that referenced this pull request Jul 4, 2015
@xerial xerial merged commit c274695 into msgpack:v07-develop Jul 4, 2015
@xerial xerial mentioned this pull request Jul 5, 2015
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.

4 participants