Re-think error swallowing for de-vendored packages #5354 #8391
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pradyunsg
reviewed
Jun 4, 2020
|
I made it a draft PR because I was unsure about testing requirements. I will go ahead and change it to a normal PR. Thanks for the help! |
pradyunsg
approved these changes
Jun 4, 2020
|
Thanks @VikramJayanthi17! |
pradyunsg
added a commit
that referenced
this pull request
Oct 10, 2020
Revert "Merge pull request #8391 from VikramJayanthi17/error-swallow-…
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #5354 instead of swallowing an error during vending and raising an error when pip attempts to use the missing module, we now raise the error immediately without modification for better error output and development experience. I implemented the solution outlined by @pradyunsg in #5354
Regarding testing : How would the maintainers expect this to be tested?
I do not see any tests explicitly for the vendored function and am unsure the file in which this would be tested in.
One test I am considering would trigger an exception in some vendored package and then asserts in the test that an exception is raised. Another would be to call vendored on a non-existent module or one with missing dependencies and check the error.