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

Introduce `Rails.gem_version` #14101

Merged
merged 1 commit into from Mar 5, 2014
Merged

Conversation

@sikachu
Copy link
Member

sikachu commented Feb 18, 2014

This method returns a Gem::Version object, which is useful when performing a comparison between versions. This was originally introduced as .version by @charliesome in #8501, but got reverted in
#10002 since it was not backward compatible.

Also, updating template for rake update_versions.

@guilleiguaran
Copy link
Member

guilleiguaran commented Feb 18, 2014

/cc @jeremy

@robin850
Copy link
Member

robin850 commented Feb 24, 2014

Excuse me if I'm wrong but existing libraries are relying on #version to ensure compatibility between several versions of Rails, no ?

@sikachu
Copy link
Member Author

sikachu commented Feb 24, 2014

@robin850 Yes, and that's why #8501 was reverted. I've stated that already in the PR description.

That's why I'm introducing .gem_version. Going forward, we will want people to rely on .gem_version to do the comparison.

Note that Rails.version is still there, as a string, after this commit.

@robin850
Copy link
Member

robin850 commented Feb 24, 2014

Note that Rails.version is still there, as a string, after this commit.

Ah sorry, missed that part. Thanks for the explanation.

@jeremy
Copy link
Member

jeremy commented Feb 24, 2014

This also changes each framework's #version to a string, breaking compatibility. Consider introducing Rails.gem_version only and leaving the others as-is.

@sikachu
Copy link
Member Author

sikachu commented Feb 25, 2014

@jeremy Then what should we do with partially-introduced #version of each framework's method? Just left them as is, or remove them? I thought they were introduced by mistake?

@jeremy
jeremy reviewed Mar 1, 2014
View changes
railties/CHANGELOG.md Outdated
@@ -1 +1,6 @@
* Introduces `Rails.gem_version` to returns a Gem::Version object, useful for
performing version comparison.

This comment has been minimized.

Copy link
@jeremy

jeremy Mar 1, 2014

Member

Introduces -> Introduce
returns -> return

We say it's useful for performing version comparison. Let's show an example. A good one is how Rails.version > '4.1.10' would be true for '4.1.2' also due to lexicographic string comparison, but would be false when comparing gem versions.

Unfortunately you can't do Rails.gem_version > '4.1.10'. You have to:

if Rails.gem_version > Gem::Version.new('4.1.10')
  # ...

and that's limited to exact version comparison. Pretty verbose and hard to discover.

Furthermore, to do more realistic version requirement checks, you'd use:

if Gem::Requirement.create('~> 4.1.2') =~ Rails.gem_version
  ...

Which you'd need some digging in RubyGems docs to even figure out.

All told, we may be better off just saying: "Introduce Rails.gem_version as a convenience method to return Gem::Version.new(Rails.version)".

It's not something most app developers would ever touch, so suggesting it's useful is a stretch, but plugin developers may take note and use it.

@jeremy
jeremy reviewed Mar 1, 2014
View changes
version.rb Outdated
STRING = [MAJOR, MINOR, TINY, PRE].compact.join(".")
module VERSION #:nodoc:
MAJOR, MINOR, TINY, PRE = Rails.version.segments
STRING = Rails.version.to_s
end

This comment has been minimized.

Copy link
@jeremy

jeremy Mar 1, 2014

Member

Why change this code? It's more confusing now. You have to understand that Rails.version will be gsubbed to a different constant, which would make the code work.

Simpler to leave the original code and add

def self.gem_version
  Gem::Version.new VERSION::STRING
end

That's true for all components and doesn't require gsubbing.

This comment has been minimized.

Copy link
@jeremy

jeremy Mar 1, 2014

Member

Also, it'd be simpler to introduce gem_version.rb in each component that's exactly the same for each and defines a self.gem_version, not self.version.

Then have version.rb in each component require_relative './gem_version' and alias it as version, except in Railties, which would define def self.version; VERSION::STRING end.

Then version.rb would never be touched by the release rake tasks, only gem_version.rb, and it wouldn't need any special cases.

@jeremy
jeremy reviewed Mar 1, 2014
View changes
Rakefile Outdated
f.write version_file.gsub(/Rails/, constants[project])
else
f.write version_file.gsub(/(Rails|self)\.version/, "\\1.gem_version")
end

This comment has been minimized.

Copy link
@jeremy

jeremy Mar 1, 2014

Member

Possible for this code to remain the same, no special case. See the comments on version.rb below.

@sikachu
Copy link
Member Author

sikachu commented Mar 5, 2014

@jeremy thanks for your awesome feedbacks. I've updated the code on this and #14103. How does it look now?

@jeremy
jeremy reviewed Mar 5, 2014
View changes
railties/CHANGELOG.md Outdated
@@ -1,3 +1,18 @@
* Introduce `Rails.gem_version` as a convenience method to return
`Gem::Version.new(Rails.version)`, suggesting more reliable way to perform

This comment has been minimized.

Copy link
@jeremy

jeremy Mar 5, 2014

Member

"suggesting more" -> "suggesting a more"

This method return `Gem::Version.new(Rails.version)`, suggesting a more
reliable way to perform version comparison.

Example:

    Rails.version #=> "4.1.2"
    Rails.gem_version #=> #<Gem::Version "4.1.2">

    Rails.version > "4.1.10" #=> false
    Rails.gem_version > Gem::Version.new("4.1.10") #=> true
    Gem::Requirement.new("~> 4.1.2") =~ Rails.gem_version #=> true

This was originally introduced as `.version` by @charliesome in #8501
but got reverted in #10002 since it was not backward compatible.

Also, updating template for `rake update_versions`.
jeremy added a commit that referenced this pull request Mar 5, 2014
Introduce `Rails.gem_version`
@jeremy jeremy merged commit 9f84c7b into rails:master Mar 5, 2014
@sikachu sikachu deleted the sikachu:master-fix-versioning-task branch Mar 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.