Skip to content

MultiJson update#558

Merged
mperham merged 1 commit intomasterfrom
multijson-update
Dec 3, 2012
Merged

MultiJson update#558
mperham merged 1 commit intomasterfrom
multijson-update

Conversation

@brandonhilkert
Copy link
Collaborator

Update MultiJson delegate methods to coincide with their future API and add supporting test.

@brandonhilkert
Copy link
Collaborator Author

Closes #558

@mperham
Copy link
Collaborator

mperham commented Dec 3, 2012

How far back in versions does the new MultiJson API go? I think that's new in 1.3.x. Does that version work with Rails 3.0? That's why Sidekiq was using the old API: because Rails 3.x has mandated different versions of MultiJson, like ~> 1.0.0. We could require >= 1.3.0 but I want to make sure we know how it will affect old Rails apps. Maybe Sidekiq 3.0 could limit support to Rails 3.2 and greater.

@brandonhilkert
Copy link
Collaborator Author

You're exactly right, those methods were added in 1.3: intridea/multi_json@e90fd6c

Also, it appears the multi_json dependency was introduced in Rails 3.1, but the version here:
https://github.com/rails/rails/blob/v3.1.0/activesupport/activesupport.gemspec#L19

Should mean that anything less than 2.0 right?

Alternatively, some conditional respond_to nonesense could check if load is available on MultiJson, or maybe it's not not worth worrying about for now. Thoughts?

@mperham
Copy link
Collaborator

mperham commented Dec 3, 2012

I don't want responds_to? and other logic hackery. So to use this, we'd need to update the version requirement in the gemspec. What should it be?

@brandonhilkert
Copy link
Collaborator Author

Part me thinks keep it under 2 for the time being is better, just to give an opportunity to flesh out anything that might change, although, the use is so minimal, it's hard to imagine any negative side effects given that they mentioned load and dump are in the 2.0 release.

gem.add_dependency                  'multi_json', '~> 1.3'

@mperham
Copy link
Collaborator

mperham commented Dec 3, 2012

I think we're ok with the current spec. We have "~> 1" right now but everyone must be on 1.3+ because we require that API.

mperham added a commit that referenced this pull request Dec 3, 2012
@mperham mperham merged commit a072e49 into master Dec 3, 2012
@brandonhilkert
Copy link
Collaborator Author

If I have MultiJson 1.2 and we added this, wouldn't it break it for me
since it would pass the gem requirement but not have those new methods?

On Dec 3, 2012, at 12:03 AM, Mike Perham notifications@github.com wrote:

I think we're ok with the current spec. We have "~> 1" right now but
everyone must be on 1.3+ because we require that API.


Reply to this email directly or view it on
GitHubhttps://github.com//pull/558#issuecomment-10941721.

@mperham
Copy link
Collaborator

mperham commented Dec 3, 2012

Right, we need a version req like ">= 1.3.0, ~> 1". Would that work?

On 2 Dec 2012, at 21:14, Brandon Hilkert notifications@github.com wrote:

If I have MultiJson 1.2 and we added this, wouldn't it break it for me
since it would pass the gem requirement but not have those new methods?

On Dec 3, 2012, at 12:03 AM, Mike Perham notifications@github.com wrote:

I think we're ok with the current spec. We have "~> 1" right now but
everyone must be on 1.3+ because we require that API.


Reply to this email directly or view it on
GitHubhttps://github.com//pull/558#issuecomment-10941721.

Reply to this email directly or view it on GitHub.

@brandonhilkert
Copy link
Collaborator Author

I think >= 1.3.0 should be enough. Sent PR.

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.

2 participants