-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
657fc8f
to
0724210
Compare
0724210
to
82cc564
Compare
Changes Unknown when pulling 82cc564 on sferik:replace-multi_json-with-json into * on aws:master*. |
@coveralls This is not helpful at all. |
@sferik unhelpful bot is unhelpful :( |
@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 💀. |
@sferik I didn't delete anything, maybe @trevorrowe or @awood45 did? |
I threaded to report them (on Twitter), so maybe @coveralls did. |
Continuing the discussion from: jmespath/jmespath.rb#6 (comment)
What I am considering:
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. |
@sferik I've addressed this in a branch that is being worked for This provides the shim we need to allow users to leverage Oj for performance while maintaining only the minimum interfaces. |
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 thejson
gem is part of the Ruby standard library. I have replaced themulti_json
dependency with ajson
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 ofjson
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 onmulti_json
.I have also added Ruby 2.2 to the Travis CI matrix for testing.