-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Make candidate download even lazier #9522
Conversation
9bed036
to
2e70ec0
Compare
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.
Pretty sure that this needs a 21.0.1.
/cc @pfmoore
Um, why? I can see that it's an improvement, but (a) is it a fix for a regression, and (b) why is it urgent enough to need an emergency release? My criteria for a bugfix release are:
Once this change is no longer in draft status, I'll review it and make a call on a 21.0.1. At the moment, this, and a possible packaging release are the two candidates, and neither is in a state to land yet. And the longer the delay, the more chance that releasing from master won't be possible (at which point I become a lot harder to convince - I simply don't have the free time to play with merge conflicts). (It's worth noting that the packaging release doesn't meet my criteria above - so I'm already bending my own rules. Don't push me too far 🙂) |
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've desk-checked this and it looks OK to me.
It looks like there will be a packaging release in the next few days, so 21.0.1 is likely to happen. So the question "does this need a 21.0.1" is no longer relevant - but someone needs to ensure this is merged before the release. I'm likely to do the release this weekend (assuming packaging gets released) and I'm not going to delay 21.0.1 for another week for this, nor am going to do a 21.0.2... |
I think (unnecessary) double downloads if you pass |
Well, that's easy. :) |
Oh, it does fix that one? I hadn't worked out why this addressed that problem. Definitely a bug worth fixing, my point was that I don't think it was necessarily urgent enough to warrant a 21.0.1 (it's "only" an extra download after all). But that's irrelevant now, so that's good. |
This iterator is used the package is not already installed. Candidates | ||
from index come later in their normal ordering. |
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.
Minor typo. I believe when is missing –" ... is used when the ..."
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.
A pull request would be welcomed :)
Fix #9516.