bpo-8978: improve tarfile.open error message when lzma / bz2 are missing#24850
Merged
miss-islington merged 1 commit intoApr 27, 2021
Merged
Conversation
|
This PR is stale because it has been open for 30 days with no activity. |
pablogsal
reviewed
Apr 27, 2021
pablogsal
reviewed
Apr 27, 2021
Member
There was a problem hiding this comment.
I am ok with this, but this is going to create a hell of a lot of cycles, so I prefer to manually delete the list with all the exceptions once error_msgs is done to alleviate this a bit.
Contributor
Author
There was a problem hiding this comment.
I actually thought about the reference cycle problem -- that's why these are strings and not exception objects -- I don't think there's actually any cycles added by this PR ? unless you're hinting at something I don't know about!
Member
There was a problem hiding this comment.
I don't think there's actually any cycles added by this PR ?
Oh, my bad, I was reading this as keeping the errors in the list instead of the string representation. Maybe renaming errors to error_messages? If you prefer not to do that is fine, I think we can go as it is as well.
Contributor
Author
There was a problem hiding this comment.
ah yeah that would be better -- but then what to call the joined variable 🤔 (to avoid having a variable change type)
Member
There was a problem hiding this comment.
the joined variable
final_error_msg ?
Member
There was a problem hiding this comment.
Notice that even if it contains the summary of multiple error messages is a single error message at the end.
Maybe you can call it summary_error_msg?
Contributor
Author
There was a problem hiding this comment.
went with error_msgs and error_msgs_summary -- thanks for the suggestion!
pablogsal
approved these changes
Apr 27, 2021
Member
pablogsal
left a comment
There was a problem hiding this comment.
LGTM
I left a minor nit, tell me what you prefer. We can land it afterwards
befb454 to
873961c
Compare
Contributor
|
@asottile: Status check is done, and it's a success ❌ . |
Contributor
|
@asottile: Status check is done, and it's a success ✅ . |
Contributor
|
Sorry, I can't merge this PR. Reason: |
Contributor
|
@asottile: Status check is done, and it's a success ✅ . |
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
https://bugs.python.org/issue8978
Automerge-Triggered-By: GH:pablogsal