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

Support venv --prompt #1570

Merged
merged 20 commits into from
Feb 19, 2024
Merged

Support venv --prompt #1570

merged 20 commits into from
Feb 19, 2024

Conversation

methane
Copy link
Contributor

@methane methane commented Feb 17, 2024

Summary

This PR adds the --prompt option to venv subcommand.

The default behavior for uv venv is to create a virtual environment in the current directory with .venv name. This is different from venv / virtualenv where a user always needs to provide the virtual environment path. This allows us to define our own behavior in the default scenario (uv venv). We've decided to use the current directory's name in that case.

Workflows:

Command Virtual Environment Name Prompt
uv venv .venv (default) Current directory name
uv venv project project project
uv venv --prompt . .venv Current directory name
uv venv --prompt foobar .venv foobar
uv venv project --prompt foobar project foobar

Fixes #1445

Test Plan

This is my first Rust code and I don't know how to write tests yet.
I just checked the behavior manually:

$ cargo build
$ mkdir t
$ cd t
$ ../target/debug/uv venv -p 3.11
$ rg -w t .venv/bin/acti*
.venv/bin/activate.csh
13:setenv VIRTUAL_ENV '/Users/inada-n/work/uv/t/.venv'
20:if ('t' != "") then
21:    setenv VIRTUAL_ENV_PROMPT 't'
23:    setenv VIRTUAL_ENV_PROMPT "$VIRTUAL_ENV:t:q"
38:    # in which case, $prompt is undefined and we wouldn't

.venv/bin/activate
48:VIRTUAL_ENV='/Users/inada-n/work/uv/t/.venv'
59:    VIRTUAL_ENV_PROMPT="t"

.venv/bin/activate.fish
61:set -gx VIRTUAL_ENV '/Users/inada-n/work/uv/t/.venv'
73:if test -n 't'
74:    set -gx VIRTUAL_ENV_PROMPT 't'

.venv/bin/activate.ps1
40:if ("t" -ne "") {
41:    $env:VIRTUAL_ENV_PROMPT = "t"

.venv/bin/activate.nu
6:# but then simply `deactivate` won't work because it is just an alias to hide
35:    let virtual_env = '/Users/inada-n/work/uv/t/.venv'
50:    let virtual_env_prompt = (if ('t' | is-empty) {
53:        't'

crates/uv/src/main.rs Outdated Show resolved Hide resolved
crates/uv/src/main.rs Outdated Show resolved Hide resolved
zanieb
zanieb previously requested changes Feb 17, 2024
Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Thanks for your pull request! I have some comments to start.

I'm not sure what the best way to test this is either, you can see our current venv tests at https://github.com/astral-sh/uv/blob/main/crates/uv/tests/venv.rs — I'd look into adding a snapshot there.

Here's a resource about our snapshotting tool: https://insta.rs/docs/cli/

@zanieb zanieb added the enhancement New feature or request label Feb 17, 2024
@zanieb
Copy link
Member

zanieb commented Feb 17, 2024

Also congrats on your first Rust pull request ❤️

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Thanks for creating this PR!

I was just playing around with venv / virtualenv when I saw this PR. That helped me understand what the original behavior is which means you don't need to fiddle around with other tools ;)

There are a few changes required to be compatible with the --prompt flag from other tools.

From the implementation side, I'm not exactly sure but you might want to play around with either using Option<&str> or Option<String> type for the prompt parameter in the gourgeist::create_bare_venv function. This is because we need to get the prompt value from the current working directory if it's Some(".") which could lead into ownership problems. Feel free to ping me if you face any, I'm happy to help :)

crates/uv/src/main.rs Outdated Show resolved Hide resolved
crates/uv/src/main.rs Outdated Show resolved Hide resolved
crates/gourgeist/src/main.rs Outdated Show resolved Hide resolved
crates/gourgeist/src/bare.rs Outdated Show resolved Hide resolved
crates/gourgeist/src/bare.rs Outdated Show resolved Hide resolved
@methane
Copy link
Contributor Author

methane commented Feb 17, 2024

When name is specified

~/work/uv/t (git)-[support-prompt] % ../target/debug/uv venv -p 3.11 hoge
Using Python 3.11.7 interpreter at /opt/homebrew/opt/python@3.11/bin/python3.11
Creating virtualenv at: hoge
~/work/uv/t (git)-[support-prompt] % grep prompt hoge/pyvenv.cfg
prompt = hoge

When name is omitted (.venv)

~/work/uv/t (git)-[support-prompt] % ../target/debug/uv venv -p 3.11
Using Python 3.11.7 interpreter at /opt/homebrew/opt/python@3.11/bin/python3.11
Creating virtualenv at: .venv
~/work/uv/t (git)-[support-prompt] % grep prompt .venv/pyvenv.cfg
prompt = t

@zanieb zanieb mentioned this pull request Feb 18, 2024
@MichaReiser
Copy link
Member

Nice. We now also support batch files. Could you add the same logic to the activation.bat file?

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

I like the default behavior that you proposed. With that in mind, this is the final set of review comments and then it should be good to go.

It's mainly a simplification of using Option to avoid using is_empty for every entry in pyvenv.cfg. This is to keep the invariant that other entries are always going to be written to the file while prompt is optional.

Thanks for your patience!

crates/gourgeist/src/bare.rs Outdated Show resolved Hide resolved
crates/uv/src/main.rs Outdated Show resolved Hide resolved
crates/uv/src/main.rs Outdated Show resolved Hide resolved
crates/gourgeist/src/bare.rs Outdated Show resolved Hide resolved
crates/gourgeist/src/bare.rs Outdated Show resolved Hide resolved
crates/uv/src/main.rs Outdated Show resolved Hide resolved
methane and others added 3 commits February 19, 2024 21:48
Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
@dhruvmanila
Copy link
Member

@methane Thanks a lot for initiating this effort! I made some small tweaks at the end to accommodate Micha's recommendation:

  1. Expanding the CLI flag documentation
  2. Using a custom enum Prompt
  3. Updated the PR description to list down the workflow

Congrats on landing your first PR and welcome to Astral! 🥳

@dhruvmanila dhruvmanila merged commit 9efbc1f into astral-sh:main Feb 19, 2024
7 checks passed
@methane methane deleted the support-prompt branch February 20, 2024 04:58
@frague59
Copy link

Great job guys !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: Support uv venv --prompt
5 participants