Skip to content
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

[2020-resolver] List downloaded distributions before exiting #8709

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

McSinyx
Copy link
Contributor

@McSinyx McSinyx commented Aug 5, 2020

This unifies the behavior of pip download (listing downloaded distributions before exiting) and fixes GH-8696.

@chrahunt
Copy link
Member

chrahunt commented Aug 5, 2020

I think it is probably reasonable to move successfully_downloaded to RequirementPreparer._prepare_linked_requirement. I would push back a little on getting rid of needs_more_preparation, because:

  1. The scope of needs_more_processing is essentially RequirementPreparer, you don't need to look outside it to understand its impact
  2. successfully_downloaded doesn't critically affect processing, just information display. Now that it is doing both it is harder to reason about.
  3. it's not even clear to me that successfully_downloaded is needed (can we just print "Processed (all reqs)" in pip download?), and if we get rid of it then there's no need to merge it with anything :)

@McSinyx McSinyx force-pushed the successfully-downloaded branch from 926d65a to d6e6ed2 Compare August 6, 2020 09:51
This unifies the behavior of pip download for both legacy and new
resolvers.  InstallRequirement.successfully_download is no longer needed
for this task and is thus retired.
@McSinyx McSinyx force-pushed the successfully-downloaded branch from d6e6ed2 to 4f210f3 Compare August 6, 2020 09:52
@McSinyx McSinyx changed the title Set InstallRequirement.successfully_download in RequirementPreparer [2020-resolver] List downloaded distributions before exiting Aug 6, 2020
@McSinyx
Copy link
Contributor Author

McSinyx commented Aug 6, 2020

Thank you for the thoughtful comment, indeed successfully_downloaded only exists for historical reasons. I've adapted the patch accordingly.

@uranusjr
Copy link
Member

uranusjr commented Aug 11, 2020

I wonder if this is a good chance to change the output format as well. The current space-delimited format is very difficult to parse, both visually or by a program.

The same actually applies to the “successfully installed” message for pip install as well. Would a PR changing it to, say, one package on a line, be welcomed?

@pradyunsg
Copy link
Member

Separately, yes.

@McSinyx
Copy link
Contributor Author

McSinyx commented Aug 12, 2020

@uranusjr, I've filed the ticket above to further discuss how we might want to improve it. BTW since you are here, may I have a review from you as well?

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

I am a little (very little) worried about dropping the if req_to_install.editable part, but can’t find a code path that makes a difference. I’ll err on the side of cleanup and agree with the logic change. We can always fix the condition if that turns out to not work.

@pradyunsg pradyunsg merged commit a3fd424 into pypa:master Aug 17, 2020
@McSinyx McSinyx deleted the successfully-downloaded branch August 17, 2020 13:43
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip download does not conclude downloaded requirements with new resolver
4 participants