-
Notifications
You must be signed in to change notification settings - Fork 187
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
Fix context manager for LocalCommand.popen() #495
Conversation
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.
Looks good if CI passes. Thank you!
@@ -23,6 +23,16 @@ | |||
SDIR = os.path.dirname(os.path.abspath(__file__)) | |||
|
|||
|
|||
class TestLocalPopen: |
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.
Can you stick a Python 3 guard here, please?
Also, this can just be a function, rather than a class in a function (up to you).
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.
Sorry I have to ask for Python 3 guards in 2020, but plumbum still supports Python 2.6. :/ But hey, we might be able to drop Python 2.6 after November 2020 (RHEL 6)!
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.
It it okay like this? Or do you want me to put a run-time check on the actual methods?
(I don't know what would be appropriate behaviour then, I guess just raising NotImplemented
instead of AttributeError
?)
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.
This is fine, adding features to Python 2 is not needed, it was an error before too, I believe.
Thanks! |
Thank you, that was quick! |
Adding tests speeds up the process considerably. ;) |
This fixes a regression introduced in 108342a.
It appears that this commit tried to implement a transparent proxy object for
subprocess.Popen
. Unfortunately, special method calls (e.g. of context manager methods by thewith
statement) generally circumvent the__getattribute__
machinery and perform direct lookup.This leads to an exception when calling LocalCommand.popen() within a with statement:
This broke some code of mine. I have not found any documentation regarding this change, so I assume dropping the context manager functionality was an accident.
This PR restores the context manager functionality for
local[...].popen()
calls.