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

Remove leading dashes in venv names #2399

Closed

Conversation

dnoetzel
Copy link
Contributor

This is designed to fix issue #439 without causing a breaking change; it does so by removing only leading slashes, instead of prohibiting slashes anywhere in the virtualenv name, as the original issue proposed.

I slightly changed how Project._sanitize() works, but it's not the only potential solution. Another option would be to change how do_create_virtualenv() calls pew; a double dash could be used to separate options from positional arguments and then even a virtualenv name that started with a dash would work. That change would look something like this:

--- a/pipenv/core.py
+++ b/pipenv/core.py
@@ -914,7 +914,6 @@ def do_create_virtualenv(python=None, site_packages=False):
             '-m',
             'pipenv.pew',
             'new',
-            project.virtualenv_name,
             '-d',
             '-a',
             project.project_directory,
@@ -932,6 +931,10 @@ def do_create_virtualenv(python=None, site_packages=False):
         err=True,
     )
     cmd = cmd + ['-p', python]
+
+    if not project.is_venv_in_project():
+        cmd = cmd + ['--', project.virtualenv_name]
+
     # Actually create the virtualenv.
     with spinner():
         try:

However, it seems to me that changing the name sanitizing method is a better choice for the following reasons:

  1. It's easier and faster to test
    Checking a string in a unit test is a fast and easily understood operation. While admittedly the context for the test could be lost if its broader purpose is not made clear, that seems like a decent tradeoff when considering the other option. Changing how pew is invoked in pipenv necessitates a slow integration test that relies on calling external code. Furthermore, it would require either extensive changes to the _PipenvInstance and TemporaryDirectory classes to allow specifying a temp dir with a defined name (think /tmp/-dir-with-leading-dash) or a lot of messy one-off setup and teardown logic.
  2. It potentially covers other edge cases
    Should virtualenv_name be passed to other external commands, options and positional args would not need to be separated by the double dash. Given that a bug of this nature would be stubborn and hard to find, this PR has the potential save a lot of time by preventing other issues before they are introduced.

If the name given to a virtualenv starts with a dash, it can be
misinterpreted as an argument when passed to external commands,
such as when pew is used create a new virtual environment.

Since pipenv has not been able to create a virtual environment in
a directory whose name begins with a dash, there are no deployments
to break with this change.

Fixes pypa#439
@uranusjr
Copy link
Member

It might be an even better idea to avoid invoking Pew altogether. The project name itself is not the problem, supplying it into the command line is. If we skip that, everything would just work.

@techalchemy
Copy link
Member

That was what I was going to suggest as well. We can just call the pew function I would imagine. I was going to suggest it but I was neck deep in that other PR

Also thanks for putting this together and especially for taking the time to include a failing test. That makes our lives so much easier for exploring options and tossing ideas around and concretely finding out if they work. So just want to let you know, we appreciate the help!

@dnoetzel
Copy link
Contributor Author

Closing in favor of #2415.

@dnoetzel dnoetzel closed this Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants