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

convert between msgpack and json via cJSON library #870

Merged
merged 1 commit into from
May 24, 2020

Conversation

ygj6
Copy link
Collaborator

@ygj6 ygj6 commented May 14, 2020

  • Pack json string to msgpack format data.
    int msgpack_pack_jsonstr(msgpack_packer *pk, const char *jsonstr)

  • Convert msgpack format data to json string.
    int msgpack_object_print_jsonstr(char *buffer, size_t length, const msgpack_object o)

@codecov-io
Copy link

Codecov Report

Merging #870 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #870   +/-   ##
=======================================
  Coverage   81.16%   81.16%           
=======================================
  Files          86       86           
  Lines        6419     6419           
=======================================
  Hits         5210     5210           
  Misses       1209     1209           

@redboltz
Copy link
Contributor

It seems that you added not only json example for C but also some convenient functions.
I think that they are nice to have.
I'm not familiar with the word "intact". I've checked it using dictionary and understand it.
Do you have any other candidate words for that ?
If "intact" is the best one, I apply it. (I'm not a native English speaker:)

@redboltz
Copy link
Contributor

I think that msgpack_pack_str_with_body could be a better name. Because msgpack_pack_str is used for packing STR header and msgpack_pack_str_body is used for packing STR body. The newly introduced function is both STR header and body. The name msgpack_pack_str_with_body mean that directly.
What do you think?

@ygj6
Copy link
Collaborator Author

ygj6 commented May 24, 2020

I have another idea:

msgpack_pack_str() => msgpack_pack_str_head()
msgpack_pack_str_body() => msgpack_pack_str_body()
msgpack_pack_str_intact() => msgpack_pack_str()/msgpack_pack_str_with_body()/msgpack_pack_str_easy()/...

bin, ext are the same as str.

@redboltz
Copy link
Contributor

If I design the API from the scratch, the following functions are better as you mentioned.
Personally, I don't like easy it is too subjective word for me ;) .

msgpack_pack_str() => msgpack_pack_str_head()/msgpack_pack_str_header()
msgpack_pack_str_body() => msgpack_pack_str_body()
msgpack_pack_str_intact() => msgpack_pack_str()

However, I want to avoid breaking change because it require code updating to many users.

@ygj6
Copy link
Collaborator Author

ygj6 commented May 24, 2020

Indeed, msgpack_pack_str() is an old funtion, changing it will cause ABI to be incompatible with previous versions. Maybe we can change it in a major version upgrade in the future.

Now I'll use msgpack_pack_str_with_body().

@codecov-commenter
Copy link

Codecov Report

Merging #870 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #870   +/-   ##
=======================================
  Coverage   81.16%   81.16%           
=======================================
  Files          86       86           
  Lines        6419     6419           
=======================================
  Hits         5210     5210           
  Misses       1209     1209           

@redboltz
Copy link
Contributor

Thank you for understanding and updating the code.

@redboltz redboltz merged commit 766afb9 into msgpack:master May 24, 2020
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