Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed #875: Loosen multi_json version. #877

Merged
merged 1 commit into from Apr 25, 2012

Conversation

Projects
None yet
3 participants
Contributor

nirvdrum commented Apr 25, 2012

If I could get another set of eyes to either review the code or at least verify it works, I'd appreciate that. I ran the tests with multi_json 1.2.0 and 1.3.2 and didn't seem to have any problems.

One change introduced that I'm not wild about is we use to lazy require 'multi_json'. I now require it optimistically in 'fog/core/json.rb', but this file gets loaded everywhere, so we effectively always load multi_json. We could relax that, but I liked caching the results of the :respond_to? call, since introspecting on every call would incur considerably more overhead than requiring once. I'm open to suggestions if anyone has a way to blend the two approaches.

Owner

geemus commented Apr 25, 2012

@nirvdrum - Looks great, thanks for handling this.

geemus added a commit that referenced this pull request Apr 25, 2012

@geemus geemus merged commit edf1c60 into master Apr 25, 2012

@danp danp commented on the diff Apr 26, 2012

lib/fog/core/json.rb
@@ -16,5 +18,29 @@ def self.sanitize(data)
end
end
+ # Do the MultiJson introspection at this level so we can define our encode/decode methods and perform
+ # the introspection only once rather than once per call.
+
+ if MultiJson.respond_to?(:dump)
+ def self.encode(obj)
+ MultiJson.encode(obj)
@danp

danp Apr 26, 2012

Member

Should this be MultiJson.dump? Same for the respond_to(:load) case using MultiJson.load below.

@nirvdrum

nirvdrum Apr 26, 2012

Contributor

Indeed. Not sure how I bungled it, but fixed in 282ee0f. Thanks for reviewing.

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