Proposal: Implement dump/load #36

Closed
sferik opened this Issue Mar 14, 2012 · 15 comments

Comments

Projects
None yet
9 participants
Member

sferik commented Mar 14, 2012

If we implemented dump and load to replace encode and decode respectively, MultiJSON could be used as a drop-in serializer for ActiveRecord objects.

I also happen to prefer dump/load over encode/decode because they are more consistent with other standard Ruby serialization libraries including Marshal, YAML, and, yes, JSON.

Obviously, renaming encode and decode is a hardcore breaking change 💣 that will require a major version bump and at least one minor release that supports both methods and contains deprecation warnings. I'd propose a very responsible transition but I believe this is a change worth making.

However, before I start writing code, I'd like to solicit feedback from anyone using this gem.

If you're not familiar with custom coders for serialized attributes in Rails 3.1 (and even if you are), I'd highly recommend watching @tenderlove's 2011 RailsConf keynote, starting around the 16 minute mark (through at least the 23rd minute): http://youtu.be/kWOAHIpmLAI?t=16m

:1:, this kind of thing is what duck typing is all about. dump / load is what I'd expect.

qrush commented Mar 14, 2012

Whatever you do, just please be aware activesupport depends on this gem...so do this for a 2.0 release. http://rubygems.org/gems/activesupport

Member

sferik commented Mar 14, 2012

@qrush Agreed. As I said above:

Obviously, renaming encode and decode is a hardcore breaking change 💣 that will require a major version bump and at least one minor release that supports both methods and contains deprecation warnings.

Owner

mbleigh commented Mar 14, 2012

:1: I'm on board for this. I did "encode" and "decode" in the first place because that's what the ActiveSupport JSON serializer used.

amiel commented Mar 15, 2012

👍

raggi commented Mar 27, 2012

👍

Member

sferik commented Mar 27, 2012

Okay, it seems like everyone is in agreement on this. Now that I've pushed version 1.2, I'm going to start implementing this transition in 1.3 (i.e. support for dump and load plus deprecation warnings for encode and decode). Just to reiterate, this will be an entirely backwards-compatible release.

Also, while I have your attention, I'd be interested to get your feedback on: #39

@ghost ghost assigned sferik Apr 14, 2012

@sferik sferik closed this in e90fd6c Apr 14, 2012

The problem with this change is that as a library author I now need to force my users to 1.3.1 +, severely restricting the other libraries they can use my library alongside, or I have to just leave my code using encode/decode and users have to deal with a bunch of noisy deprecations spewing out that they can't do anything about.

:( :( :(

I'd suggest that encode/decode should continue to work for a decent while before being deprecated and then removed. That would make the upgrade process a lot less painful.

I completely agree with @jonleighton

Member

sferik commented Apr 22, 2012

@jonleighton That's a false dichotomy. You could also feature-detect. That's what we're doing in Rails: https://github.com/rails/rails/pull/5896/files

That's a fair point. Still, I don't see any reason to deprecate encode/decode immediately. It's great to support load/dump, but why not just let encode/decode linger for a while before deprecating it? Otherwise every user of multi json has to add this feature detect code...

I'm with @jonleighton here: I chose to use MultiJson in VCR precisely because I didn't want to do the feature-detection dance to figure out what JSON libraries are available. MultiJson was nice because I could use it and it worked fine with virtually any set of gems users are using (including multiple versions of MultiJson, yajl, json-gem, etc).

With this change, your forcing me to go back to doing the feature-detection dance, which goes against the very reason I chose to use MultiJson in the first place. I don't want VCR users to be getting warnings from MultiJson but I also don't want to force them to be on any particular version (since they may have other gems that require particular versions).

I posted this on twitter and it's only partially tongue-in-cheek:

Methinks we need a new multi_multi_json gem that provides a single unified API that works properly with multiple multi_json versions.

Please consider keeping the encode/decode interface w/o deprecation warnings. What harm does it cause? Just make them aliases for dump/load. Then those of us who are using MultiJson precisely to avoid the feature-detection dance can continue to use the unified API that MultiJson has always provided.

Member

sferik commented Apr 28, 2012

@jonleighton @myronmarston Okay, I'm convinced. Shortly, I will release MultiJson 1.3.4, which removes the deprecation warnings and aliases encode/decode to dump/load.

I'm not going to make any promises about whether encode/decode will continue to be supported in 2.0, but the warnings seem like they're doing more harm than good.

@sferik thank the gods

Great, thanks!

sferik added a commit that referenced this issue Apr 28, 2012

Remove deprecation warnings
Some good points were made by @jonleighton and @myronmarston in #36
following the 1.3 release. While some or all of these aliased methods
may still be removed or deprecated in version 2, it doesn't seem right
to subject users of libraries that depend on multi_json to deprecation
warnings that they can't do anything about.

@mloughran mloughran referenced this issue in pusher/pusher-http-ruby May 6, 2012

Closed

Fix deprecation warnings for multi_json. #21

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