-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Isolate, reuse PackageFinder best candidate logic #5971
Isolate, reuse PackageFinder best candidate logic #5971
Conversation
e5bfbdf
to
df68f3b
Compare
Question / observation: to make this much more easily testable (and easier to understand, etc), it looks like you might be able to convert both of Does that look right to you? |
I considered that approach as well, but opted for the least-effort one in the end. I’ll try it instead. The call in |
df68f3b
to
9b20265
Compare
Coming back to this (I was busy with other things lately), I think your previous version might have been better. I don't know for sure because I can't see it any longer, but it might be because of the awkwardness you mentioned -- without a similar gain to offset it. My suggestion of passing the return value of |
Yes, indeed |
1b7026f
to
0f9f23a
Compare
I’ve worked on this a bit, and have a sort of design problem in refactoring. An easy way to implement this would be to expose three methods:
Now both
If you want all candidates, use the first one. If you only want to know what to install, use the second one. No way to go wrong. The problem is, however, The current version is what I consider a middle ground. The three methods are kept, but marked as internal (except I don’t think this is a good implementation, but is the correct design. If this can be merged, I will submit another purely refactoring PR to deal with the duplicate code, probably by introducing an internal structure to hold the three internal values. |
Looking at the current version, I think it does what's needed well enough; and @uranusjr's suggested plan moving forward sounds fair to me. I'll defer to @cjerdonek since he's been more involved here than I have. :) |
Soft ping :) |
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.
Thanks for pinging / reviving this. I appreciate your work and thoughtful approaches.
Some comments.
src/pip/_internal/index.py
Outdated
if applicable_candidates: | ||
best_candidate = max(applicable_candidates, | ||
key=self._candidate_sort_key) | ||
best_candidate = self._select_best_candidate(applicable_candidates) |
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 read and appreciate your comments describing your thinking on this. Thanks. While I think I understand why you did it, on balance it seems to me like it would be better to avoid copying the exact three lines right next to each other, even if this is something you plan to fix later. It seems like it would be simpler and clearer and less error-prone to have them both go through the same function.
Two easily implementable alternatives, both of which I'm sure you've already thought of, and either of which I'd be okay with--
-
You could add a
return_intermediate_values=False
argument thatfind_requirement()
could passTrue
for. PassingTrue
would cause the return value to be a tuple rather than a single value. It's ugly, but would get the job done (and at least it wouldn't affectpip_version_check()
's invocation). -
Kind of like
subprocess.run()
's API that returns aCompletedProcess
object, you could return an object instead of a single value. You could even have two variants:analyze / plan / calculate_best_candidate()
that returns an object containing auxiliary info, and a simpler wrapper functionfind_best_candidate()
thatpip_version_check()
could use that returns just the final value. As for (1), the latter would make it sopip_version_check()
would be unaffected by the extra info.
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 CompletedProcess
model sounds like a good idea :D In retrospect it is not unlike the original tuple solution, but with a better interface.
src/pip/_internal/utils/outdated.py
Outdated
all_candidates = finder.find_all_candidates("pip") | ||
if not all_candidates: | ||
try: | ||
candidate = finder.find_best_candidate("pip", SpecifierSet()) |
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.
It seems like the caller shouldn't have to know about SpecifierSet
objects to call find_best_candidate()
. What about making that argument optional?
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 more fond of being explicit :) Convenience is not as important here since the API is internal (anyone wanting to understand the this line need to know about specifiers anyway), and always requiring a SpecifierSet
reduces the chance of forgetting to pass one when you should.
Would finder.find_best_candidate("pip", specifier=None)
be an acceptable middle ground?
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 concur omitting the specifier should be okay, since finder.find_best_candidate("pip")
is clear enough in terms of communicating what's happening.
I'm okay with all three options on the table, only slightly preferring the form I've stated here.
65cae6e
to
1e85cf7
Compare
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
Split out how PackageFinder finds the best candidate, and reuse it in the self version check, to avoid the latter duplicating (and incorrectly implementing) the same logic.
1e85cf7
to
ea1d5ac
Compare
This looks even cleaner than I expected! Thanks so much @cjerdonek 😀 (Also rebased to keep the bot happy.) |
I have some comments after the latest changes. |
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.
Lots of changes in this re-write of the PR, so some new comments.
if best_installed: | ||
# We have an existing version, and its the best version | ||
logger.debug( | ||
'Installed version (%s) is most up-to-date (past versions: ' | ||
'%s)', | ||
installed_version, | ||
', '.join(sorted(compatible_versions, key=parse_version)) or |
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.
It looks like parse_version
got dropped in the new code.
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.
This is merged into the above variable. I have made this into a function instead, with additional comments to make the usage clearer (hopefully).
def __init__( | ||
self, | ||
candidates, # type: List[InstallationCandidate] | ||
specifier, # type: specifiers.BaseSpecifier |
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.
Again, I think specifier should be optional (defaulting to the empty SpecifierSet
).
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 made the specifier
argument optional in finder.find_candidates()
, but kept it required in FoundCandidates
constructor.
Also fix the missing parse_version call, and add comment for it.
ce8a021
to
2882042
Compare
Are there things I could do to push this forward? Should I change |
Hi @uranusjr, sorry! I actually just spent time going over this again an hour or so ago. I had been wanting to go over it again for a while but only just now got to it. I had some further minor thoughts, but I think it looks good enough. I was planning to merge it first thing tomorrow. Thanks for your thought and work on this and for your patience. |
PS - if there were further tweaks you wanted to do, you could always do that in a later PR. (And if it's a smaller / easier PR, I promise reviews can happen way faster.) It's only when there's more going on in a PR that it slows things down. |
Regarding |
Ah, I see what you mean, good point. I’ll work on improving that after this gets merged, then. |
Thanks again for your work on this, @uranusjr! 👍Can you double-check that the issues this closes are closed? |
def _format_versions(cand_iter): | ||
# This repeated parse_version and str() conversion is needed to | ||
# handle different vendoring sources from pip and pkg_resources. | ||
# If we stop using the pkg_resources provided specifier. |
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.
PS - also, here's a code comment that got chopped off that can be fixed in a subsequent commit.
I believe all related issues/PRs (at least those mentioned) are closed 🍿 |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
My take on #5175. This turns out to be more complicated than I imagined. I decided to make the outdated check follow how pip finds versions for requirements passed by the user:
allow_all_prereleases
is True, consider prereleases. (not applicable for self version check)I also tweaked the code in
find_requirement
a little to refactor out some duplicate formatting.Fix #5175, close #5928.