-
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
Refactor the freeze command #10410
Refactor the freeze command #10410
Conversation
d595aae
to
f39a639
Compare
f39a639
to
1739e43
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.
A few minor stylistic suggestions. The rest LGTM.
news/10410.feature.rst
Outdated
@@ -0,0 +1,2 @@ | |||
Allways fallback to reporting the editable project location when pip freeze |
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.
typo:
Allways fallback to reporting the editable project location when pip freeze | |
Always fallback to reporting the editable project location when pip freeze |
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.
nit-ish: I would rephrase this to something like:
pip freeze
will now ...
I find it easier to skim, when we mention the "thing-that-we-affected" at the start of the message.
else: | ||
# name==version requirement | ||
req = _format_as_name_version(dist) | ||
comments = [] |
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.
nit: I'd put this at the start of the else.
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.
Feel free to merge, once you fix the typo.
Drop the "awaiting response" label, if you wanna include this in the 21.3 release and want me to do a review before merging. :)
At the beginning this function returns if not dist.editable. So returning editable=False after that does not make sense. Return an editable with the file system location instead when there is any issue getting VCS information.
It is always true.
1739e43
to
6ac3ffe
Compare
This is a followup to and sits on top of #10249.
This slightly changes the output of pip freeze in case the detection of the VCS reference of an editable install fails.
Before it returned a non editable requirement in some VCS error situations.
Now it always returns an editable requirement, but pointing to the local directory when the VCS could not be determined for any reason.
This is more consistent, avoids information loss, and greatly simplifies the logic.