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

JSON instead of Marshalling #77

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@Gargron

Gargron commented May 30, 2011

This is a very little fix but it allows non-ruby applications to interact with the sessions data in redis. Either accept this or make JSON optional, and explain how to specify the option in Sinatra in the docs. Thank you!

@SpectateSwamp

This comment has been minimized.

Show comment
Hide comment
@SpectateSwamp

SpectateSwamp Jun 1, 2011

Wow, this is dumb, somebody close this. It breaks the specs and is slow as molasses.

Wow, this is dumb, somebody close this. It breaks the specs and is slow as molasses.

@Gargron

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Jun 1, 2011

I tried getting help about this before, but without success, so I went and did it myself. As this is JSON encoding, it will break some non-standard objects. I had only the Rack::Session:Redis stuff in mind when doing this, so I don't know how it would work with the cache and the rest of the functions. As I therefore stated in my pull request, it would be nice if someone went over my idea making JSON optional.

Gargron commented Jun 1, 2011

I tried getting help about this before, but without success, so I went and did it myself. As this is JSON encoding, it will break some non-standard objects. I had only the Rack::Session:Redis stuff in mind when doing this, so I don't know how it would work with the cache and the rest of the functions. As I therefore stated in my pull request, it would be nice if someone went over my idea making JSON optional.

@carlzulauf

This comment has been minimized.

Show comment
Hide comment
@carlzulauf

carlzulauf Jun 3, 2011

I like the concept of using JSON for interoperability with non-ruby apps. Marshal.load isn't the fastest thing in the world either, so not sure this should be disregarded just because it's slow. They are both pretty slow. Only trying to parse a string as JSON when str.starts_with?('{','[') might mitigate some performance concerns.

This should be fleshed out more before submitting a pull though, with rspec cases for it and maybe some changes which makes the marshaling method used configurable.

Maybe the best approach is 3-tiered, where strings are NOT serialized, Array/Hash is serialized into JSON, and other ruby objects are serialized using Marshal.

I like the concept of using JSON for interoperability with non-ruby apps. Marshal.load isn't the fastest thing in the world either, so not sure this should be disregarded just because it's slow. They are both pretty slow. Only trying to parse a string as JSON when str.starts_with?('{','[') might mitigate some performance concerns.

This should be fleshed out more before submitting a pull though, with rspec cases for it and maybe some changes which makes the marshaling method used configurable.

Maybe the best approach is 3-tiered, where strings are NOT serialized, Array/Hash is serialized into JSON, and other ruby objects are serialized using Marshal.

@jodosha

This comment has been minimized.

Show comment
Hide comment
@jodosha

jodosha Jun 5, 2011

Member

Marshalling could be slow but it's the backed in Ruby solution for serialization that just works. I'm figuring out solutions to store objects in a more efficient way, but this patch breaks all the specs and it isn't backward compatible with existing real world applications.

Member

jodosha commented Jun 5, 2011

Marshalling could be slow but it's the backed in Ruby solution for serialization that just works. I'm figuring out solutions to store objects in a more efficient way, but this patch breaks all the specs and it isn't backward compatible with existing real world applications.

@jodosha jodosha closed this Jun 5, 2011

@carlzulauf

This comment has been minimized.

Show comment
Hide comment
@carlzulauf

carlzulauf Jun 6, 2011

I really like the concept of using JSON when appropriate. I am submitting another pull that doesn't break ANY specs or existing applications while successfully allowing for marshaling to/from JSON when appropriate (arrays, hashes).

I really like the concept of using JSON when appropriate. I am submitting another pull that doesn't break ANY specs or existing applications while successfully allowing for marshaling to/from JSON when appropriate (arrays, hashes).

@carlzulauf

This comment has been minimized.

Show comment
Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment