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

Support Gem::Version properly #1024

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@knewter

knewter commented Mar 24, 2013

I haven't tested this with rails < 4-master, but this fixes breakage where Rails.version now returns a Gem::Version and these comparisons fail there. This is the offending commit afaict: rails/rails@c07e151

Josh Adams
@jnicklas

This comment has been minimized.

Collaborator

jnicklas commented Mar 25, 2013

That seems weird. Comparing string sizes like this seems incorrect to me. For example:

>> "3.1.2.beta1" > "3.1.2"
=> true
@knewter

This comment has been minimized.

knewter commented Mar 25, 2013

That's an excellent point, though I have no idea what to_f did with that. I didn't even consider that case, though it's obvious now. How is Gem::Version intended to be compared?

@abotalov

This comment has been minimized.

Collaborator

abotalov commented Mar 25, 2013

Here's even more weird example:

'3.1.2' > '3.1.10' # => true 

What about <=>:
Rails.version >= Gem::Version(3.0)

@jnicklas

This comment has been minimized.

Collaborator

jnicklas commented Mar 25, 2013

@abotalov if Rails previously returned something other than a Gem::Version that might break on older versions.

@abotalov

This comment has been minimized.

Collaborator

abotalov commented Mar 25, 2013

@jnicklas What about:

if Rails.version.is_a?(String)
  Gem::Version.new(Rails.version) >= Gem::Version.new(3.0)
else
 Rails.version >= Gem::Version.new(3.0)
end
@jnicklas

This comment has been minimized.

Collaborator

jnicklas commented Mar 25, 2013

I'm not even sure what it was before, was it really a string?

@abotalov

This comment has been minimized.

Collaborator

abotalov commented Mar 25, 2013

@jnicklas

1.9.3p286 :015 > Rails.version
 => "3.2.11" 
@eval

This comment has been minimized.

eval commented Mar 26, 2013

How about:

Gem::Version.new(Rails.version) >= Gem::Version.new(3)

This is compatible with Rails.version being a string or Gem::Version

@elabs-dev elabs-dev closed this in 78e3a48 Mar 26, 2013

jnicklas added a commit that referenced this pull request Mar 26, 2013

Make sure we are comparing gem versions, closes #1024
Due to rails/rails#8501, we can no longer case the Rails version to a float. This version should hopefully work on both Rails 4 master and on Rails 3.
@jnicklas

This comment has been minimized.

Collaborator

jnicklas commented Mar 26, 2013

I cherry picked this onto 2.0_stable and released it as 2.0.3 together with another tweak. A simple bundle update capybara should now fix this issue.

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