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

Report more meaningful errors on why virtualenv creation failed #556

Merged
merged 1 commit into from Jul 16, 2017

Conversation

Projects
2 participants
@vlaci

vlaci commented Jul 15, 2017

virtualenv fails if the destination directory contains unicode characters or spaces. A more descriptive error is printed if it's likely that that was the case.
As long as the upstream problem exists #121 cannot be fixed.

Open questions:

  • Should hard-fail even before executing virtualenv if the destination directory contains unsupported characters? -- I choosed not to in order to not block support of such paths if upstream is fixed
  • Should check for unsupported characters and only print error message about them if they are found? -- I decided not to, because testing this part of the code would be very hard and I don't like the idea to introduce logic without tests. I could abstract away the error reporting of venv.update() calls and test it in isolation if needed.

Thanks for contributing a PR!

Here's a quick checklist of what we'd like you to think off:

  • Make sure to include one or more tests for your change;
  • if an enhancement PR please create docs and at best an example
  • Add yourself to CONTRIBUTORS;
  • make a descriptive Pull Request text (it will be used for changelog)
Vaskó László
session: report more meaningful errors on why virtualenv creation failed
virtualenv fails if the destination directory contains unicode
characters or spaces. A more descriptive error is printed if it's
likely that that was the case.
I didn't precheck the directory path for not supported characters
because if this problem is fixed by upstream we wouldn't want
execution to fail.

@vlaci vlaci force-pushed the vlaci:issue-121 branch from 3845759 to da05e97 Jul 15, 2017

@vlaci

This comment has been minimized.

vlaci commented Jul 16, 2017

#203 is also related

@obestwalter obestwalter merged commit c9bde7b into tox-dev:master Jul 16, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@obestwalter

This comment has been minimized.

Member

obestwalter commented Jul 16, 2017

Thanks @vlaci :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment