Skip to content

Let's try this again...#149

Merged
mperham merged 2 commits intosidekiq:masterfrom
sferik:multi_json
Apr 22, 2012
Merged

Let's try this again...#149
mperham merged 2 commits intosidekiq:masterfrom
sferik:multi_json

Conversation

@sferik
Copy link
Contributor

@sferik sferik commented Apr 22, 2012

Use Object#respond_to? to determine which MultiJson API to use.

Use `Object#respond_to?` to determine which MultiJson API to use.
@jc00ke
Copy link
Contributor

jc00ke commented Apr 22, 2012

I'd much rather see us use a method like Sidekiq.decode all over the place & check the version in there. Not a fan of introducing those conditionals everywhere.

@mperham
Copy link
Collaborator

mperham commented Apr 22, 2012

I'm supposed to add this ugly hack because you don't want to alias dump to encode?

@sferik
Copy link
Contributor Author

sferik commented Apr 22, 2012

@mperham Encode is aliased to dump, it just throws warnings. If you don't want warnings, you need to feature-detect.

@mperham
Copy link
Collaborator

mperham commented Apr 22, 2012

Yeah, I'm saying there's no reason for the warnings. Just support both pairs. dump/load and encode/decode are both perfectly reasonable names.

These methods perform MultiJson feature detection and can be removed
after this library's MultiJson dependency is upgraded to ~> 2.0.
@sferik
Copy link
Contributor Author

sferik commented Apr 22, 2012

I've cleaned up the patch, per @jc00ke's suggestion.

mperham added a commit that referenced this pull request Apr 22, 2012
@mperham mperham merged commit c852d7e into sidekiq:master Apr 22, 2012
@mperham
Copy link
Collaborator

mperham commented Apr 22, 2012

Much better, thanks.

@jc00ke
Copy link
Contributor

jc00ke commented Apr 22, 2012

Yes, much better. Thanks @sferik!

@sferik
Copy link
Contributor Author

sferik commented Apr 22, 2012

My pleasure.

When MultiJson 2.0 is released and you're ready to upgrade, you should be able to safely remove Sidekiq.[dump|load]_json and then run these commands:

git ls-files *.rb | xargs sed -i '' 's/Sidekiq.load_json/MultiJson.load/g'
git ls-files *.rb | xargs sed -i '' 's/Sidekiq.dump_json/MultiJson.dump/g'

@jc00ke
Copy link
Contributor

jc00ke commented Apr 22, 2012

I would actually prefer to leave as is and only change what's inside Sidekiq.[dump|load]_json if necessary. No reason to create a greater surface area, and in case we ever want to change/update in the future it'll be easy.

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.

3 participants