-
Notifications
You must be signed in to change notification settings - Fork 476
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
Ensure command names in search path of run() or spawn() commands do not match folders. #37
Conversation
fix for incorrect directory match
That makes sense. Could you add a test for this in |
if platform.system() == "Linux": | ||
exe = "ls" | ||
elif platform.system() == "Windows": | ||
exe = "cmd.exe" |
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.
No need for this check - pexpect only works on Unix-y systems anyway.
@lcm337 , I'll be happy to merge this if you fix the couple of things I pointed out - the change to command lookup, and the unnecessary Windows test code. |
Will do.
|
Ping. |
elif platform.system() == "Windows": | ||
exe = "cmd.exe" | ||
else: | ||
assert True, "Unknown platform" |
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.
weird, wouldn't this never fail?
>>> assert True, 'never fails'
>>>
just got to this to say i like the isdir() check. the test is kindof ok. This could be done with mock -- or more simply, overriden.
outputs
|
does sh have the same bug? Interesting. |
just submitted a similar fix to sh. I can see @lcm337 timed out regarding your suggestions, will pick it up. |
Closed by pull #58 |
fix for incorrect directory match