Conversation
Codecov Report
@@ Coverage Diff @@
## master #398 +/- ##
==========================================
+ Coverage 90.22% 90.22% +<.01%
==========================================
Files 8 8
Lines 2526 2528 +2
==========================================
+ Hits 2279 2281 +2
Misses 247 247
Continue to review full report at Codecov.
|
| REDIRECTION_OUTPUT = '>' | ||
| REDIRECTION_APPEND = '>>' | ||
| REDIRECTION_CHARS = [REDIRECTION_PIPE, REDIRECTION_OUTPUT] | ||
|
|
tleonhardt
left a comment
tleonhardt
left a comment
There was a problem hiding this comment.
Strongly consider adding back the shlex.split call when calling subprocess.Popen. In general it should probably be there unless Popen is called with shell=True, but that has other side effects.
| # We want Popen to raise an exception if it fails to open the process. Thus we don't set shell to True. | ||
| try: | ||
| self.pipe_proc = subprocess.Popen(shlex.split(statement.pipe_to), stdin=subproc_stdin) | ||
| self.pipe_proc = subprocess.Popen(statement.pipe_to, stdin=subproc_stdin) |
There was a problem hiding this comment.
I don’t think you want to drop the shlex.split call within the first argument of the call to subprocess.Popen.
There was a problem hiding this comment.
statement.pipe_to gets used in exactly one place, and it's right here. in parsing.py we took a list of shlexed tokens and joined them with a space to generate statement.pipe_to, then we split them apart to pass it to the subprocess command. I just changed statement.pipe_to to have the list of tokens, instead of a string.
Is your concern passing a string to subprocess.Popen()? I agree that would be a concern, but we are passing a list of arguments, just like we were before.
Or is there another concern?
There was a problem hiding this comment.
We might want to add shell=True here anyway and switch to a string so that we can support a command like:
(Cmd) help | wc > "count it.txt"
There was a problem hiding this comment.
Oh, if statement.pipe_to is already a list then we are good.
And if you think there are advantages in using shell=True we can try it. For some things it can be helpful, but other times it has messed us up.
There was a problem hiding this comment.
I think for now I'll just leave it as it is with the list. That will be no change in current pipe functionality. If we want to revisit the shell=True option later, we can.
3 participants
This closes #381