Add custom venv name support

[darelf]

check environment variable PIPENV_CUSTOM_VENV_NAME

Resolves #1226

[matteius]

@darelf I am wondering about this with respect to managing multiple projects--since this current patch seems to indicate that an environment variable is set to name the project's virtualenv it would not scale well beyond one project. I am wondering if it would be better to have a variable like PIPENV_VENV_NAME_SHORT that is just a boolean that defaults to false, but when set to true would just use the sanitized name which should match the project directory? This would scale well and would also resolve the original request, too. Is there a use case for trying to have the virtualenv name be different from the project directory name?

Other than that, once we settle on the approach here, we will want to document the change with a news fragment and update any documentation. Also tests, we should cover the change with at least some unit test.

[darelf]

@matteius I had originally done just such a thing. However, in my own workflow I realized that the pain point for me was similar to the one @ianyoung in the thread on that issue. Namely, I would have a project directory, then one or more subdirectories in which where the python project(s).

My original looked something like this:

sanitized, encoded_hash = self._get_virtualenv_hash(self.name)
if os.getenv("PIPENV_VENV_NAME_SHORT"):
  return sanitized

This is certainly easier if your Pipfile is in a directory with the name you want for the venv, and it solved the original request for #1226 but didn't actually solve "custom" venv names. In the case of a directory structure with a common base name, and a "frontend" and "backend" subdirectory with separate Pipfile, you'd want something like "myproject-frontend" and "myproject-backend". And that doesn't solve that you might actually want the hash appended in that case.

To satisfy custom naming, it might be better to have a scheme like this:

PIPENV_CUSTOM_VENV_NAME - overrides the venv name
PIPENV_VENV_SHORT_NAME - eliminates the hash value
PIPENV_VENV_BASE_NAME - sets the base name of the venv

This would mean the code for that function would look something like this:

custom_name = os.getenv("PIPENV_CUSTOM_VENV_NAME")
if custom_name:
    return custom_name
names = []
base_name = os.getenv("PIPENV_VENV_BASE_NAME")
if base_name:
  names.append(base_name)
sanitized, encoded_hash = self._get_virtualenv_hash(self.name)
suffix = ""
if self.s.PIPENV_PYTHON:
    if os.path.isabs(self.s.PIPENV_PYTHON):
        suffix = "-{0}".format(os.path.basename(self.s.PIPENV_PYTHON))
    else:
        suffix = "-{0}".format(self.s.PIPENV_PYTHON)

names.append(sanitized)
if not os.getenv("PIPENV_VENV_SHORT_NAME"):
  names.append(encoded_hash)
return "-".join(names) + suffix

[darelf]

I guess I should outline what that would do.

Assume a directory called "frontend" with your Pipfile

PIPENV_VENV_SHORT_NAME would give you frontend as your name
PIPENV_VENV_BASE_NAME=projectname would give you projectname-frontend-<hash> as your name
Both together would give you projectname-frontend as your name

[matteius]

@darelf I think the logical complexity of that method has grown beyond what is good maintainability with the current iteration. I like where you have went with it, but do we really need PIPENV_VENV_BASE_NAME - sets the base name of the venv. ?? That seems overly complex when also adding a PIPENV_CUSTOM_VENV_NAME that could be whatever you want. What if you just had:
PIPENV_CUSTOM_VENV_NAME - overrides the venv name (and takes precedence)
PIPENV_VENV_SHORT_NAME - eliminates the hash value from the generated name

[matteius]

Also the thing about PIPENV_CUSTOM_VENV_NAME - overrides the venv name (and takes precedence) Since you can't simultaneously have that variable set to two different things, is do you plan on only having one virtualenv managed by pipenv on the system? Is this a Docker container use case?

[darelf]

So, is the thinking that the custom name could include the base name if desired? That makes a lot of sense and reduces the complexity. You're already setting a variable with a custom name...

So, if you have 2 subdirectories under /projectname/ one as /projectname/frontend/ and one as /projectname/backend/, then you would have to set the PIPENV_CUSTOM_VENV_NAME differently for each in order to get projectname-frontend and projectname-backend. But if you have PIPENV_VENV_BASE_NAME=projectname then you only set that variable once.

PIPENV_CUSTOM_VENV_NAME would basically be for a user who wants precise control. With the other 2 variables, the desire for it fades away to a very niche case. I would think just going with the other 2 (PIPENV_VENV_BASE_NAME and PIPENV_VENV_SHORT_NAME) would satisfy both the original request and the subsequent comments.

[darelf]

Apologies for the extra commits.... we don't use pull requests at work, so I was unaware it tracked changes.

[matteius]

Apologies for the extra commits.... we don't use pull requests at work, so I was unaware it tracked changes.

@darelf It is fine, if we eventually merge it we can use the Squash and Merge options which will take the current change delta and make a single commit out of it.

So, is the thinking that the custom name could include the base name if desired? That makes a lot of sense and reduces the complexity. You're already setting a variable with a custom name...

Exactly, and keep in mind that the method will need tests. Adding two custom environment variables may be a tough sell, but three for this would probably be a non-starter.

With the other 2 variables, the desire for it fades away to a very niche case. I would think just going with the other 2 (PIPENV_VENV_BASE_NAME and PIPENV_VENV_SHORT_NAME) would satisfy both the original request and the subsequent comments.

I think that makes sense, except perhaps for the naming of PIPENV_VENV_BASE_NAME which is really an override of the prefix of the virtualenv name. Maybe it should be named PIPENV_VENV_PREFIX_NAME? Keep in mind that the reason for the nit picking is we should really hone in on the best names and best implementation here because whatever is chosen will need to be supported into the future for all users.

[darelf]

I have no problem with the "nit picking". A lot of these are just "here's a name that works for me that I thought of in 0.25 seconds" so I'm not married to any of that.

PREFIX actually makes more sense, and follows logically.

As an additional note: I mainly use the join() to avoid return-ing multiple times in a function. If it feels unreadable, I can do basic string concatenation instead. Seeing sanitized + "-" + encoded_hash + suffix made my eyes go cross, because the code includes the "-" in the suffix string....

[matteius]

As an additional note: I mainly use the join() to avoid return-ing multiple times in a function. If it feels unreadable, I can do basic string concatenation instead. Seeing sanitized + "-" + encoded_hash + suffix made my eyes go cross, because the code includes the "-" in the suffix string....

Ok that sounds like a solid argument, thanks for putting thought into this. I think leaving it a join seems reasonable.

[matteius]

@darelf Can you work on some unit test(s) for this and add a feature news fragment to start the documentation updates? Once you do that I can run the workflow. We should also consider updating the actual documentation about these environment variables.

[darelf]

I'll start looking at tests, etc. for this as I get time.

[oz123]

I am sorry for the late review here.
The original intent of the issue was nailed by the first commit, which includes:

os.getenv("PIPENV_CUSTOM_VENV_NAME")

The following commits added complexity, which also always adds a hyphen to the VENV name. If you have a scenario with different projectname-frontend and projectname-backend, you can also solve the issue with:

PIPENV_CUSTOM_VENV_NAME=projectname-frontend

And in another shell, with:

PIPENV_CUSTOM_VENV_NAME=projectname-backend

Easy to use when running anything like a modern terminal with tabs or tmux ... This also works for any IDE.

This will make this feature easy to understand for users as well as maintainers.

[oz123]

@darelf thank you for pushing this issue forward. This is now implemented via #5203 .