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

feat: add .compare(a, b), .rcompare(a, b) #12

Merged
merged 2 commits into from Jun 21, 2017
Merged

Conversation

keithamus
Copy link
Member

@keithamus keithamus commented Jun 19, 2017

I've exposed the compare function which give access to the raw sorting algo, useful when sorting on more than one dimension.

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CI failure in Ruby 1.8 looks like it might be unrelated to the changes in this branch. But just to be sure, care to look into why it might be failing?

}

static VALUE
rb_version_compare_r(VALUE rb_self, VALUE rb_version_a, VALUE rb_version_b)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're exposing a low-level function, I don't think it makes sense to expose an alternate rcompare() function that simply returns a negated result of compare(). If the user needs to write low-level comparison code, then they can negate the result of compare() themselves, and they don't strictly need rcompare(). Does that make sense?

If we got rid of rcompare, we wouldn't have the need for the internal rb_version_compare_1 function either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I simply added it for completeness.

@keithamus
Copy link
Member Author

Looks like the build is failing because of the version of Gem - which is used in the tests to ensure we sort the same version set that Gem would. The last passing build installed Gem 2.4.5 ($ gem --version == 2.4.5) while the very next build started using Gem 2.0.17 ($ gem --version == 2.0.17) I'm guessing somewhere between those two versions, Gem was able to parse version strings like 2.0-rc2

@mislav
Copy link
Contributor

mislav commented Jun 19, 2017

@keithamus Thanks for investigating! It's really a bummer that rubygems got downgraded from 2.4 to 2.0 between builds 😕

@keithamus keithamus force-pushed the feat-add-compare branch 2 times, most recently from d81f4a2 to 99a9b3d Compare June 20, 2017 10:12
@keithamus
Copy link
Member Author

@mislav build now passing 🎉! Had to add before_install step to upgrade gem, bundler.

@mislav
Copy link
Contributor

mislav commented Jun 20, 2017

Thanks for figuring this out!

However, gem update --system; gem install bundler are kind of slow to perform on every CI build, for every ruby version, even if they are not necessary (they are just needed for 1.8.7). Could we either just do this on 1.8.7, and avoid doing it for other versions, or could we simply skip the offending test at runtime if RUBY_VERSION == '1.8.7'?

@keithamus
Copy link
Member Author

Okay noted. I'll probably go with something like the latter option.

@@ -21,6 +21,10 @@ def test_sorts_versions_correctly

def test_sorts_versions_like_rubygems
versions = %w(1.0.9.b 1.0.9 1.0.10 2.0 3.1.4.2 1.0.9a 2.0rc2 2.0-rc1)
if (Gem.rubygems_version < Gem::Version.new('2.1.0'))
# Old versions of RubyGems cannot parse semver versions like `2.0-rc1`
versions.pop()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So rather than skip the test entirely, I just removed the offending version that cannot be parsed with older rubygems versions. So the test still runs and works - it just isn't as comprehensive for old versions.

@keithamus
Copy link
Member Author

Yay it actually passes now @mislav!

@mislav mislav merged commit f2d9c93 into master Jun 21, 2017
@mislav mislav deleted the feat-add-compare branch June 21, 2017 11:52
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.

None yet

2 participants