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

conservative check for known exceptions in subprocess stderr. #3463

Merged
merged 3 commits into from
Apr 2, 2019

Conversation

AdamGold
Copy link
Contributor

@AdamGold AdamGold commented Jan 20, 2019

Thank you for contributing to Pipenv!

The issue

#2553

The fix

Conservative checks of known exceptions when subprocess returns output.

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 #.

@AdamGold AdamGold force-pushed the bugfix/catch-command-errors branch 4 times, most recently from 1d67834 to d5519fc Compare January 25, 2019 09:32
@AdamGold
Copy link
Contributor Author

This is a UX improvement PR. Do you guys think there is a better way of doing it?

Copy link
Member

@techalchemy techalchemy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey, sorry for the delay -- made some suggestions for how to generalize this improvement a bit more -- thanks a ton for putting in the effort on this as it's really a big help and not likely something we will ever do otherwise

pipenv/core.py Outdated
for partition in (part
for e, part in known_exceptions.items() if e in c.err):
known_exceptions_partition = c.err.rpartition(partition)
c.err = "{} {}".format(known_exceptions_partition[1], known_exceptions_partition[2])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't known_exceptions_partition[1] and known_exceptions_partition[2] just be PermissionError: <message> in this case? I like where this is headed but I feel like it might belong in exceptions.py. Also note that it really isn't appropriate to manipulate c.err -- it is the stored stderr stream output from a subprocess that has already or is currently executing. We really should only ever manipulate how we interpret and display the result.

Possibly we should have some helper method like this:

KNOWN_EXCEPTIONS = {
    "PermissionError": "Permission denied:"
}

def prettify_exc(exc):
    matched_exceptions = [k for k in KNOWN_EXCEPTIONS.keys() if k in exc]
    errors = []
    for match in matched_exceptions:
        exc, exc_name, error = exc.rpartition(KNOWN_EXCEPTIONS[match])
        errors.append("{0} {1}".format(exc_name, crayons.red(decode_for_output(error))))
    else:
        errors.append("{0}".format(decode_for_output(exc)))
    return "\n".join(errors)

And then the exceptions themselves can share a dictionary of known exceptions and use prettify_exc when environments.is_verbose() is False

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea. I have pushed the prettify_exc implementation.

@techalchemy techalchemy added the Status: Awaiting Update ⏳ This issue requires more information before assistance can be provided. label Mar 11, 2019
@AdamGold AdamGold force-pushed the bugfix/catch-command-errors branch 2 times, most recently from ae8ad82 to 62d3ac4 Compare March 11, 2019 07:39
@techalchemy
Copy link
Member

Sorry for being incredibly slow on this, but this is looking pretty good to me... I am excited to get things refactored in this direction.

Thanks for taking care of this!

@techalchemy techalchemy added Type: Enhancement 💡 This is a feature or enhancement request. Priority: Low This item is low priority and may not be looked at in the next few release cycles. Status: Accepted ✔️ This item has been reviewed and accepted. Status: Rebase Required This item is out of date and must be caught up. labels Apr 1, 2019
@AdamGold AdamGold force-pushed the bugfix/catch-command-errors branch 4 times, most recently from 62d3ac4 to f02676c Compare April 2, 2019 12:45
@AdamGold
Copy link
Contributor Author

AdamGold commented Apr 2, 2019

@techalchemy, Awesome. Rebased to master.

@AdamGold AdamGold force-pushed the bugfix/catch-command-errors branch from f02676c to cd98e13 Compare April 2, 2019 12:49
catch known errors in stderr and display them correctly
@AdamGold AdamGold force-pushed the bugfix/catch-command-errors branch from cd98e13 to 2d75f1a Compare April 2, 2019 12:49
@techalchemy techalchemy merged commit 6c62d23 into pypa:master Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low This item is low priority and may not be looked at in the next few release cycles. Status: Accepted ✔️ This item has been reviewed and accepted. Status: Awaiting Update ⏳ This issue requires more information before assistance can be provided. Status: Rebase Required This item is out of date and must be caught up. Type: Enhancement 💡 This is a feature or enhancement request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants