Skip to content
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

WIP: Raise exception from subprocess.check_output before actually calling … #3458

Closed
wants to merge 0 commits into from

Conversation

AdamGold
Copy link
Contributor

@AdamGold AdamGold commented Jan 18, 2019

Thank you for contributing to Pipenv!

WIP

The issue

  1. Errors if WORKON_HOME is not writable could be better #2553
  2. errors from subprocess not being caught
  3. misc.py called sys.stderr with 3 arguments instead of 1.

The fix

call subprocess.check_output before Popen and raise exceptions.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

@jxltom jxltom changed the title raise exception from subprocess.check_output before actually calling … WIP: Raise exception from subprocess.check_output before actually calling … Jan 18, 2019
@AdamGold
Copy link
Contributor Author

What custom exception do you think I should raise when the return code from the command is non zero (==failed)?

@frostming
Copy link
Contributor

Changes to vendors should be submitted to upstream as well.

@frostming frostming added the Type: Vendored Dependencies This issue affects vendored dependencies within pipenv. label Jan 18, 2019
@AdamGold AdamGold closed this Jan 20, 2019
@AdamGold AdamGold force-pushed the bugfix/catch-command-errors branch from 3f58396 to c4341a7 Compare January 20, 2019 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Vendored Dependencies This issue affects vendored dependencies within pipenv.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants