Skip to content

Fix incomplete clean up of odd python requirements #5647

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

Merged
deivid-rodriguez merged 1 commit into from
Sep 8, 2022
Merged

Fix incomplete clean up of odd python requirements #5647

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
2 changes: 1 addition & 1 deletion python/lib/dependabot/python/requirement.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ def convert_python_constraint_to_ruby_constraint(req_string)
return nil if req_string == "*"

req_string = req_string.gsub("~=", "~>")
req_string = req_string.gsub(/(?<=\d)[<=>].*/, "")
req_string = req_string.gsub(/(?<=\d)[<=>].*\Z/, "")

if req_string.match?(/~[^>]/) then convert_tilde_req(req_string)
elsif req_string.start_with?("^") then convert_caret_req(req_string)
Expand Down
11 changes: 11 additions & 0 deletions python/spec/dependabot/python/requirement_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,17 @@
end
end

context "with multiple operators after the first" do
let(:requirement_string) { ">=2.0<2.1<2.2" }
# Python ignores operators after the first!
it { is_expected.to eq(Gem::Requirement.new(">=2.0")) }
Copy link
Copy Markdown
Member

@jeffwidman jeffwidman Sep 12, 2022

Choose a reason for hiding this comment

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

Curious why you added a new test case rather than modifying the existing test case from 45f5b77#diff-606e746adf06604fa9039dd903280cffd0c3ee1485e2d54ad5e13932279a019aR40-R44 ?

Is there a scenario you envision where it's useful to test 2 operators that wouldn't be caught by testing 3 operators?

Copy link
Copy Markdown
Contributor Author

@deivid-rodriguez deivid-rodriguez Sep 12, 2022

Choose a reason for hiding this comment

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

Normally I prefer adding separate specs over modifying existing ones, mostly because I don't want to lose any coverage and many times I don't feel I can tell for sure whether that would be the case if I modify the spec. I also think it makes patches more focused, although it normally introduces some duplication and overhead. In this case it would probably be fine to change the existing spec, although it would be a bit weird to only test three operators instead of two? I think testing both cases illustrates that there's a bit more complexity when handling more than two.

Copy link
Copy Markdown
Member

@jeffwidman jeffwidman Sep 13, 2022

Choose a reason for hiding this comment

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

Sounds reasonable.


context "separated with a comma" do
let(:requirement_string) { ">=2.0,<2.1,<2.2" }
it { is_expected.to eq(Gem::Requirement.new(">=2.0", "<2.1", "<2.2")) }
end
end

context "with an array" do
let(:requirement_string) { ["== 1.3.*", ">= 1.3.1"] }
its(:to_s) do
Expand Down