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
Improve shell #298
Improve shell #298
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 mostly good. Can you remove the issue reference please? Also, I'd like to wait for the functional testing in the build to be working prior to merging this. Unfortunately, test_functional isn't running yet.
examples/subprocess_shell.py
Outdated
@@ -31,3 +34,21 @@ def Popen(*args, **kwargs): | |||
subprocess.Popen(command, shell=True) | |||
|
|||
subprocess.Popen('/bin/ls && cat /etc/passwd', shell=True) | |||
|
|||
# Issue #157 |
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.
You can remove references to bugs/issues. We can always use git or git blame to look up changes based on what issue.
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.
Done
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.
@ericwb are your change recommendations satisfied now (if so, happy to merge)
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 looks mostly good to go. Just had one question.
@@ -5,6 +5,9 @@ | |||
def Popen(*args, **kwargs): | |||
print('hi') | |||
|
|||
def __len__(self): |
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.
Is this related to this patch? Is this needed?
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.
Any object with length 0 is False, I set this in the test because this will not be detected as False, so is a known false positive.
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'll merge for now.
Done #157