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.

Sign up for GitHub

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

Undo requirement sorting #2652

Merged
1 commit merged into from
Feb 21, 2019
Merged

Undo requirement sorting #2652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions lib/rubygems/requirement.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ def initialize(*requirements)
@requirements = [DefaultRequirement]
else
@requirements = requirements.map! { |r| self.class.parse r }
sort_requirements!
end
end

Expand All @@ -144,7 +143,6 @@ def concat(new)
new = new.map { |r| self.class.parse r }

@requirements.concat new
sort_requirements!
end

##
Expand Down Expand Up @@ -186,7 +184,7 @@ def as_list # :nodoc:
end

def hash # :nodoc:
requirements.hash
requirements.sort.hash
end

def marshal_dump # :nodoc:
Expand Down Expand Up @@ -295,14 +293,6 @@ def fix_syck_default_key_in_requirements # :nodoc:
end
end

def sort_requirements! # :nodoc:
@requirements.sort! do |l, r|
comp = l.last <=> r.last # first, sort by the requirement's version
next comp unless comp == 0
l.first <=> r.first # then, sort by the operator (for stability)
end
end

end

class Gem::Version
Expand Down
1 change: 0 additions & 1 deletion test/rubygems/test_gem_requirement.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ def test_initialize
assert_requirement_equal "= 2", ["2"]
assert_requirement_equal "= 2", v(2)
assert_requirement_equal "2.0", "2"
assert_requirement_equal ["= 2", ">= 2"], [">= 2", "= 2"]
Copy link
Member

Choose a reason for hiding this comment

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

If this PR still fixes #2230 why we deleting this assertion?

Copy link
Member Author

@deivid-rodriguez deivid-rodriguez Feb 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

As I understand the wording of that PR, the problem was that versions 1.0.0 and 1.0 were not considered equivalent. Or similarly for the associated bundler issue, that the requirements >= 0.0.0 and >= 0 were not considered equivalent.

#2230 fixed that issue and added a bunch of assertions covering that, but at the same time started sorting the requirements and added this specific test covering that.

In this PR I'm only reverting the sorting of requirements (and thus removing this associated spec), but I'm keeping the fix for the above problem and all the tests covering that.

Copy link
Member

Choose a reason for hiding this comment

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

@segiddins want to give feedback? ^

Copy link
Member

Choose a reason for hiding this comment

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

I’m not happy to see this reverted

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate @segiddins? It's no big deal if we need to revert this PR.

end

def test_create
Expand Down
8 changes: 8 additions & 0 deletions test/rubygems/test_gem_specification.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2544,6 +2544,14 @@ def test_to_ruby_fancy
assert_equal @c1, same_spec
end

def test_to_ruby_keeps_requirements_as_originally_specified
spec = util_spec 'a', '1' do |s|
s.add_dependency 'b', ['~> 1.0', '>= 1.0.0']
end

assert_includes spec.to_ruby, '"~> 1.0", ">= 1.0.0"'
end

def test_to_ruby_legacy
gemspec1 = Gem::Deprecate.skip_during do
eval LEGACY_RUBY_SPEC
Expand Down