-
Notifications
You must be signed in to change notification settings - Fork 77
Add BackgroundCommand.communicate() #568
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
Add BackgroundCommand.communicate() #568
Conversation
Add a "cmd" attribute for better exception messages.
setrofim
left a comment
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 cannot be merged without the implementation of communicate() for all existing derivations of ConnectionBase (i.e. LocalConnection and AdbConnection).
|
@setrofim I'm not sure to understand, communicate() is a new method of BackgroundCommand, and is implemented by all its subclasses. ConnectionBase is left untouched in the operation. |
setrofim
left a comment
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.
D'oh. My bad. I was fooled by the collapsed code and read it as being added to ConnectionBase. My apologies.
|
No worries, GitHub is indeed not so smart in its folding. It only shows the previous "def ..." line in the hunk header, instead of doing something based on indentation |
Add a communicate() method in the style of Popen.communicate(). Unlike Popen.communicate, it will raise a CalledProcessError if the command exit with non-zero code.
8b157ca to
2c1bb42
Compare
|
EDIT: the comment was on the wrong PR and is irrelevant here |
Ease blocking interaction with a BackgroundCommand object, in the style of Popen.communicate()
Tested on paramiko and local target, but not android.