-
Notifications
You must be signed in to change notification settings - Fork 9
Encode strings for JSON with binaries #10
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
Conversation
@sega-yarkin I assume this can go into |
@michaelklishin Yes, I tested this even in 3.5.7 |
Can you please also submit this against the |
1 similar comment
Can you please also submit this against the |
@michaelklishin sorry for dummy question, but what did you mean under |
I now see that this particular repo doesn't have the |
It looks fine, but perhaps we should just switch to jsx or jiffy directly without having this intermediate step? |
I think it's fine to use this in 3.6.x.
|
I have some errors when trying to run the tests. While on a quick glance it seems that it's just the tests that need updating (they're matching against lists instead of binary), it's possible an issue hides there. I saw that you are new to Erlang so if it's a problem tell me and I'll update the tests directly. Thanks for the patch btw! |
@essen Thanks that you mentioned about tests, I forget about case when Erlang itself is easy to learn language (I've written several RMQ plug-ins for our environment that work well in production already), but there are many tools around the language which I haven't known yet. Can you tell me how to run the tests? :-) Thanks! |
I'm not sure what's the proper way to do it to be honest. I had to do a little trick to make it work with the included rebar. Assuming a clean repo, in the modified file there's an |
@essen Thanks for your help! Now all tests passed, yay!)) |
Merged, thanks! |
Encoding strings use only binaries instead of xmerl_ucs:from_utf8 (which converts it to list) when convert to JSON, it saves some memory and makes RabbitMQ more stable.
We haven't had any issues on production after updating to this code.