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

Replace multi_json with standard library json #792

Closed
wants to merge 2 commits into from

Conversation

sferik
Copy link

@sferik sferik commented Apr 23, 2015

As on of the authors of multi_json, I no longer use it in my own projects and no longer recommend its use to others, now that the json gem is part of the Ruby standard library. I have replaced the multi_json dependency with a json gem dependency, despite the fact that this is not strictly necessary, just to ensure that users of older versions of Ruby, which ship with an older version of json are using the latest gem version instead. This dependency could be removed.

I have opened a similar pull request against jmespath to remove the transitive dependency on multi_json.

I have also added Ruby 2.2 to the Travis CI matrix for testing.

@sferik sferik force-pushed the replace-multi_json-with-json branch from 657fc8f to 0724210 Compare April 23, 2015 07:27
@sferik sferik force-pushed the replace-multi_json-with-json branch from 0724210 to 82cc564 Compare April 23, 2015 07:29
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 82cc564 on sferik:replace-multi_json-with-json into * on aws:master*.

@sferik
Copy link
Author

sferik commented Apr 23, 2015

@coveralls This is not helpful at all.

@lsegal
Copy link
Contributor

lsegal commented Apr 24, 2015

@sferik unhelpful bot is unhelpful :(

@sferik
Copy link
Author

sferik commented Apr 24, 2015

@lsegal I was kind of hoping you wouldn’t delete all those spam messages so I could report them for abuse. Now that GitHub supports multiple pull request status checks, @coveralls bot should 💀.

@lsegal
Copy link
Contributor

lsegal commented Apr 24, 2015

@sferik I didn't delete anything, maybe @trevorrowe or @awood45 did?

@sferik
Copy link
Author

sferik commented Apr 24, 2015

I threaded to report them (on Twitter), so maybe @coveralls did.

@trevorrowe
Copy link
Member

@sferik @lsegal I deleted them as they weren't adding any value.

@trevorrowe
Copy link
Member

Continuing the discussion from: jmespath/jmespath.rb#6 (comment)

Given the small surface area required, I would like to explore removing the MultiJson dependency, while preserving the faster parsing options until at such time as Oj becomes standard.

If you’re able to achieve this, I’d encourage you to contribute this code to MultiJson.

What I am considering:

  • Add a pair of non-public utility method to the SDK. Name these method very un-originally, something like Aws.load_json and Aws.dump_json.
  • Update the SDK to use these when marshaling and unmarhsaling JSON payloads and when loading the large API definitions. Use these for performance sensitive runtime code paths.

A simple implementation of this module could be:

module Aws
  # @api private
  module Util
    class << self

      def load_json(json)
        ENGINE.load(json)
      end

      def dump_json(obj)
        ENGINE.dump(obj)
      end

      def oj_engine
        require 'oj'
        Oj
      rescue LoadError
        nil
      end

      def json_engine
        require 'json'
        JSON
      end

    end

    ENGINE = oj_engine || json_engine

  end
end

This is simplified version of what I do in place of using multi_xml: https://github.com/aws/aws-sdk-ruby/blob/master/aws-sdk-core/lib/aws-sdk-core/xml/parser.rb#L74.

trevorrowe added a commit that referenced this pull request May 8, 2015
@trevorrowe
Copy link
Member

@sferik I've addressed this in a branch that is being worked for v2.1.0 of the SDK. You can see my relevant changes here: https://github.com/aws/aws-sdk-ruby/blob/version-2.1-refactor/aws-sdk-core/lib/aws-sdk-core/json.rb#L13-L56

This provides the shim we need to allow users to leverage Oj for performance while maintaining only the minimum interfaces.

@trevorrowe trevorrowe closed this May 8, 2015
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.

4 participants