bpo-38662: Invoke pip via runpy, in ensurepip#17029
bpo-38662: Invoke pip via runpy, in ensurepip#17029pradyunsg wants to merge 1 commit intopython:masterfrom
Conversation
|
Should this get a skip-news label? |
| finally: | ||
| sys.argv[:] = backup_argv | ||
|
|
||
| assert 0, "should never reach here" |
There was a problem hiding this comment.
Why don't just write a return o raise an exception instead of the message "Should never reach here"?
There was a problem hiding this comment.
Well, they'd be functionally the same as far as I'm concerned. This is just a sanity-check, so I'm using an assert.
Anyway, if this is the problem with this PR, I'll be happy to change it.
There was a problem hiding this comment.
"raise SystemError" complaining that pip didn't exit as expected would be the way to go here.
ncoghlan
left a comment
ncoghlan
left a comment
There was a problem hiding this comment.
I was skeptical, but the draft PR has convinced me that this is a good idea.
Changes needed are the discussed SystemError, and adding a news entry with blurb.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be put in the comfy chair! |
|
@pradyunsg, please address the review comments. Thanks! |
|
I will submit a new PR with the requested changes. We agreed on this with @pradyunsg on IRC. |
|
In here: #18901 |
|
Closing in favor of #18901 |
8 participants
From an inspired idea in #16782 (comment)
https://bugs.python.org/issue38662