Skip to content

Fix incomplete clean up of odd python requirements#5647

Merged
deivid-rodriguez merged 1 commit intomainfrom
deivid-rodriguez/python-requirement-cleanup
Sep 8, 2022
Merged

Fix incomplete clean up of odd python requirements#5647
deivid-rodriguez merged 1 commit intomainfrom
deivid-rodriguez/python-requirement-cleanup

Conversation

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez commented Sep 7, 2022

In 45f5b77, we started handling odd python requirements, with multiple constraints not separated by colons.

Native behavior is to ignore anything after the first requirement, yet we were only ignoring the first one.

@pavera
Copy link
Copy Markdown
Contributor

pavera commented Sep 7, 2022 •
edited
Loading

I'm not sure how this fits in, but this PR tickled my memory. Poetry at least allows a space delimiter as well as a comma: #5363.

I've also confirmed Poetry allows no delimiter:

websockets = [
    {version = ">=9.0.1<11.0", python = ">= 3.7, < 3.10"},
    {version = "^10.0", python = "^3.10"}
]

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

deivid-rodriguez commented Sep 7, 2022

Aha! I had recently learnt that requirement format for Poetry is different than the standard, so I had assumed that we were not parsing those in the same way. But maybe we are? I will review this!

@deivid-rodriguez deivid-rodriguez self-assigned this Sep 7, 2022
@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review September 7, 2022 19:43
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner September 7, 2022 19:43
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/python-requirement-cleanup branch from 953396f to a82a8d8 Compare September 7, 2022 19:44
@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

deivid-rodriguez commented Sep 7, 2022

@pavera I had a look at this. Turns out that, while poetry accepts all three formats, the "no delimiter" format works in the standard weird way of ignoring any requirements after the first one. So this PR should be correct I believe.

The fix for #5363 should be easy, I'll work on it.

pavera
pavera approved these changes Sep 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@pavera pavera left a comment

Choose a reason for hiding this comment

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

🚀

@deivid-rodriguez
Fix Python not cleaning up weird requirements
4f4fd6f 
In 45f5b77, we started handling odd
python requirements, with multiple constraints not separated by colons.

Native behavior is to ignore anything after the first requirement, yet
we were only ignoring the first one.

This was discovered though a CodeQL alert.
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/python-requirement-cleanup branch from a82a8d8 to 4f4fd6f Compare September 8, 2022 17:19
@deivid-rodriguez deivid-rodriguez merged commit 697f8e9 into main Sep 8, 2022
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/python-requirement-cleanup branch September 8, 2022 17:34
jeffwidman
jeffwidman reviewed Sep 12, 2022
View reviewed changes
python/spec/dependabot/python/requirement_spec.rb
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.

@pavera pavera mentioned this pull request Oct 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeffwidman jeffwidman jeffwidman left review comments

@pavera pavera pavera approved these changes

Assignees

@deivid-rodriguez deivid-rodriguez

Labels

None yet

Projects

Dependabot
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@deivid-rodriguez @pavera @jeffwidman