-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-8978: improve tarfile.open error message when lzma / bz2 are missing #24850
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
bpo-8978: improve tarfile.open error message when lzma / bz2 are missing #24850
Conversation
This PR is stale because it has been open for 30 days with no activity. |
for comptype in sorted(cls.OPEN_METH, key=not_compressed): | ||
func = getattr(cls, cls.OPEN_METH[comptype]) | ||
if fileobj is not None: | ||
saved_pos = fileobj.tell() | ||
try: | ||
return func(name, "r", fileobj, **kwargs) | ||
except (ReadError, CompressionError): | ||
except (ReadError, CompressionError) as e: |
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.
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.
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.
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!
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.
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.
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.
ah yeah that would be better -- but then what to call the joined variable 🤔 (to avoid having a variable change type)
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.
the joined variable
final_error_msg
?
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.
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
?
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.
went with error_msgs
and error_msgs_summary
-- thanks for the suggestion!
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.
LGTM
I left a minor nit, tell me what you prefer. We can land it afterwards
befb454
to
873961c
Compare
@asottile: Status check is done, and it's a success ❌ . |
@asottile: Status check is done, and it's a success ✅ . |
Sorry, I can't merge this PR. Reason: |
@asottile: Status check is done, and it's a success ✅ . |
|
https://bugs.python.org/issue8978
Automerge-Triggered-By: GH:pablogsal