-
Notifications
You must be signed in to change notification settings - Fork 17
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
we need to cd to the final destination even if source fails #20
base: master
Are you sure you want to change the base?
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.
otherwise lgtm
aactivator.py
Outdated
@@ -246,23 +246,38 @@ def security_check(path): | |||
) | |||
|
|||
|
|||
def commands(commands, always=None): | |||
""" | |||
run a set of commands, stopping on the first error |
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 docstring seems false, this function does not "run a set of commands, stopping on the first error"
TEST> cd .. | ||
TEST> echo ok | ||
ok | ||
''' |
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.
When run in master, this test gives:
E AssertionError: Incorrect output.
E >>> Context:
E TEST>
E >>> Expected:
E pwd
E /tmp/buck/pytest-of-buck/pytest-13/test_broken_activate_shell1_0/venv/child-dir
E (activation failure)
E
E >>> Actual:
E pwd
E /tmp/buck/pytest-of-buck/pytest-13/test_broken_activate_shell1_0/venv
E (activation failure)
E TEST>
Finally got around to it. |
This is a rough draft. I'll add test after you confirm the approach seems sound.