Skip to content

Support addressable version 2.3 #197

Closed
sferik opened this Issue Jul 30, 2012 · 9 comments

6 participants

@sferik
sferik commented Jul 30, 2012

Currently, webmock is locked to an outdated version of addressable. As a result, webmock cannot be used with gems that depend on that latest version.

Here is a list of the changes made to addressable since version 2.2.8. There is also a CHANGELOG, which provides a summary of the changes in each release.

@amatsuda
amatsuda commented Aug 9, 2012

:heavy_plus_sign::one:

@bblimke
Owner
bblimke commented Aug 9, 2012

Unfortunately Addressable 2.3.0 breaks backwards compatibility in 2.x.x major version.

Webmock will have to monkeypatch addressable 2.3 behaviour or otherwise
or otherwise break backwards compatibility in 1.x major version.

I'll have a look at monkeypatching solution this week.

@jcf
Collaborator
jcf commented Aug 9, 2012

I've had a quick look at this and pushed up a branch where I have started to think about fixing this.

I've added Appraisal to WebMock so we can run the test suite against v2.2.x and v2.3.x of Addressable because I think it would be nice to support both versions going forward.

There are 159 failures mainly due to a change in how nested parameters are handled.

What do you think @bblimke?

@jcf
Collaborator
jcf commented Aug 9, 2012

For convenience, CI output can be found here.

@myronmarston
Collaborator

Webmock will have to monkeypatch addressable 2.3 behaviour or otherwise
or otherwise break backwards compatibility in 1.x major version.

It bothers me that WebMock reaches for monkeypatching as one of the first tools to consider. Consider that applications that use WebMock may use addressable directly as well, and if you monkey patch it, you may cause problems with their application.

Instead of monkey patching, please consider doing something like this:

class SomeWebMockClassThatUsesAddressable
  if Addressable::VERSION.split('.').first(2).join('.') == '2.3'
    def some_method_that_uses_addressable
      # code that works with addressable 2.3
    end
  else
    def some_method_that_uses_addressable
      # code that works with addressable 2.2
    end
  end
end

Or you might consider putting the addressable-dependent code into a separate file, and have two versions of the file that take care of working with the 2 versions of addressable. Then you can conditionally require one or the other based on the addressable version.

@bblimke
Owner
bblimke commented Aug 9, 2012

@myronmarston sorry, I worded it incorrectly, thank you for pointing that out.
I don't plan to change any Addressable code by any means :)
WebMock needs a copy of two methods Addressable::URI#query_values and Addressable::URI#query_values=
from Addressable version 2.2, and use them instead of using methods from 2.2

@bblimke
Owner
bblimke commented Aug 9, 2012

@jcf appraisal solution looks great. I wish it was available when I split em-http-request adapters.

@colszowka

Big :heavy_plus_sign::one: from me on this as well. Because the launchy gem requires ~> 2.3 I cannot use both webmock and launchy in the same test suite, which is a pity. I hope this can be resolved soon!

@bblimke
Owner
bblimke commented Aug 15, 2012

Fixed in version 1.8.9.

Although changes in addressable 2.3.0 deserve a new major version, they make sense.
and I consider doing the same in the future versions of WebMock. I.e it will allow to handle
multiple query params with the same name i.e a=1&a=2, which is currently not supported by WebMock (and addressable < 2.3.0),

If WebMock follows Addressable changes, then it will have to drop support for 'subscript' notation introduced in Rails i.e. a[]=1&a[]=2.

@bblimke bblimke closed this Aug 15, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.