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

Add msgpack serialization format #68

Closed
byu opened this issue Jul 19, 2011 · 5 comments · Fixed by #69
Closed

Add msgpack serialization format #68

byu opened this issue Jul 19, 2011 · 5 comments · Fixed by #69

Comments

@byu
Copy link
Contributor

byu commented Jul 19, 2011

It would be nice to include msgpack as another serialization. It would be a benefit to not have to maintain different rendering processes for json and msgpack in my app.

http://msgpack.org/

@nesquena
Copy link
Owner

That's a great idea actually, I am not opposed to adding that as another format. However, I know nothing about msgpack and I do not use it in my apps. If someone wants to write a patch to add a to_msgpack format here: https://github.com/nesquena/rabl/blob/master/lib/rabl/engine.rb#L51 I would much appreciate it.

@byu
Copy link
Contributor Author

byu commented Jul 19, 2011

I think I understand the engine well enough to attempt to add to_msgpack. It would mostly be a clone of Engine#to_json and Engine#format_json.

Question. How to go about the msg pack dependency?

There is a hard dependency, but it wouldn't be so nice for people that don't have it, or want it, installed:
s.add_dependency 'msgpack', '~> 0.4.5'

I think a soft dependency would work out nicely, but with the following warnings:

  • extra gem the user needs to be aware of in order to use.
  • to_msgpack will just error out if msgpack gem isn't installed.

Which way would you like me to proceed?

@byu byu closed this as completed Jul 19, 2011
@byu byu reopened this Jul 19, 2011
@nesquena
Copy link
Owner

Soft dependency with a note in the readme about enabling msgpack support.

@nesquena
Copy link
Owner

Also please add corresponding riot tests if you can (check out existing for inspiration). Thanks, nice to have another format enabled for RABL

@nesquena
Copy link
Owner

nesquena commented Sep 2, 2011

I have merged this in. Thanks! 4462ea8

@nesquena nesquena closed this as completed Sep 2, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants