Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update lower boundary in Python pip pyproject when bumping versions #6631 #10060
Update lower boundary in Python pip pyproject when bumping versions #6631 #10060
Changes from all commits
a740c39
bb91a7e
3936a85
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above block of specs makes sense to me 👍.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the previous outcome better here. If the latest resolvable version is 1.5.0, and user has specified >=2.0.0 in their manifest, I think either:
Either way, it seems best to leave this kind of ill defined case alone?
Also, the new solution is actually widening the requirement, not increasing it, which seems completely against the spirit of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new solution actually it is not widening it. It is setting the lower bound as the latest version and the upper bound is still following the last version by increasing. As it is mentioned in the document tried to follow the typical scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding this test case correctly, the previous requirement is
>= 2.0.0
, correct? If Dependabot sets the new range to be>= 1.5.0
, it's actually decreasing the lower bound and thus widening the requirement.This seems like an ill-defined case to me. Why would the existing range be
>= 2.0.0
if the latest version is 1.5.0? That's why it seemed reasonable to not mess with this case and call it "unfixable".That said, I just came up with a potential case when this could happen in real life: If latest version 2.0.0 is yanked, then that could make 1.5.0 the "new" latest version and make a case like this possible.
Previously Dependabot would avoid messing up with this case, and now it's downgrading the version to 1.5.0. I guess it's not as bad after all!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am strongly of the opinion that dependabot should never lower a
>=
constraint. When such a constraint is used, I believe it means one of two things:increase
strategy.widen
strategy and dependabot should not touch lower bound.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize I got the context wrong: we're talking about
increase
here. I still think a user intervention would be more appropriate here, but I am not as strongly invested as for the widen case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I do think there could be all sort of expectations here. I guess it could also happen that someone using
widen
has changed their code to support 2.0 version of some library and bumped their lower bound requirement to>=2.0
, then 2.0 is abruptly removed and user may appreciated that Dependabot "rolls back" their requirement change and let them know through a PR they need to revert code changes too to get back on track.Also, strictly speaking, for the
widen
strategy, Dependabot is also doing exactly what's advertised right? Namely, "widen the range to accommodate for the latest version". Just that in this case, the latest version is not newer than the upper bound, but older than the lower bound.In any case, I think this is exactly why I advocate for "leave this kind of case alone", because different users will have different expectations, or even the same user may want different things depending on the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do make a good point, this is a case where it's difficult to find a universal approach to fit everyone's use case. Your suggestion to leave it alone sounds sensible to me (biased though I am because that is what I've been advocating regardless).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I implemented this, my thoughts were as follows:
In real-world scenarios, this occurs when no range was specified before, when a higher version is yanked and necessitates a downgrade to a previous release, or when the strategy wasn’t set to increase when a lower bound is added. In such cases, Dependabot can adjust the lower bound accordingly.
For the increase strategy, if a lower constraint is defined to ensure compatibility, the lower bound should not be higher than the latest version since the latest version is functional for the app or library. Otherwise, it risks excluding versions that work correctly, potentially breaking the app.
I would appreciate your thoughts on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you're saying, but I still feel like it's a risky assumption to make for the widen strategy. For example, consider the case where a library detects a breaking bug in their latest release, so they yank it and then release a patched one soon after. If they do it in that order, dependabot might run in the window where the broken one was yanked but the new one not yet released. It would then change the lower bound, while it wouldn't have taken action at all if it had run a few minutes later.
If the library does not release a patched version (which is quite rare as far as I know), then action will indeed be required. But since this is such a rare occurrence, and lowering the constraint might not always be desired, I feel like it's safer to leave this as a problem for the user to solve.
That said, I'm just a bystander here, so I won't try to push my view any further than this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @sanderr here. I think Dependabot should create update PRs that work for everyone, or when that's not the case, at least allow the behavior to be configurable. This seems such an edge case that feels best to not try be too smart and leave it for the user to fix manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should Dependabot increase the upper bound simmetrically to the lower bound i.e., leave a 3 major version range in place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking it shouldn't because you will never know how upcoming versions are handled. Also here we are handling only lower bound the upper bound is working as how it was working before. Do you think we need to reconsider the upper bound in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, there's no clear solution, but
<=1.10,>=1.10
feels not right? Either one of:<=1.11,>=1.10
.<=1.10.2,>=1.10
I'd probably leave this one alone as
:unfixable
since it's unclear what the user wants in the case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous comment on the similar case on
requirement.txt
files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous comment on the similar case on
requirement.txt
files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the previous outcome better here. If the latest resolvable version is 1.5.0, and user has specified
>=2.0.0
in their manifest, I think either:Either way, it seems best to leave this kind of ill defined case alone?
Also, the new solution is actually widening the requirement, not increasing it, which seems completely against the spirit of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous comment on the similar case on
requirement.txt
files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above block of specs makes sense to me 👍.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous comment on the similar case on
requirement.txt
files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous comment on the similar case on
requirement.txt
files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above block of specs makes sense to me 👍.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous comment on the similar case on
requirement.txt
files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous comment on the similar case on
requirement.txt
files.