-
Notifications
You must be signed in to change notification settings - Fork 760
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
Print activation instructions for a venv after one has been created #1580
Conversation
crates/uv/src/commands/venv.rs
Outdated
@@ -188,5 +189,22 @@ async fn venv_impl( | |||
} | |||
} | |||
|
|||
match &platform.os() { | |||
Os::Windows => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about cmd users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can detect the current terminal type in Windows...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(or if we should just show both?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure if we can correctly/reliably detect the current terminal type in Windows, I've added in the below prompt:
Activate with:
- Powershell: .\.venv\Scripts\activate.ps1
- CMD: .\.venv\Scripts\activate.bat
and updated filters and snapshots
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexWaygood / @MichaReiser can you take over review of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe typing .venv\Scripts\activate
(omitting the file extension) should actually work on both PowerShell and CMD. (It does on my Windows machine!) If we just suggested doing that, it woudl avoid us having to figure out how to detect which Windows shell the user is using (and it's also fewer characters to have to type out ;).
If you're on CMD, it'll naturally pick up the .bat
file; if you're on PowerShell, it'll naturally pick up the .ps1
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, that definitely helps make the prompt a lot cleaner/simpler. Have updated it.
7f1c99f
to
eaf059d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
I tested locally with zsh on my mac, CMD on my Windows PC, and PowerShell on my Windows PC. All looked good! |
Summary
.venv
after it has been created.uv venv
does not suggest how to activate #1326If the platform os matches
Os::Windows
then we provide the following prompt:Otherwise we provide the following prompt:
I've added additional filters in
crates/uv/tests/venv.rs
so there shouldn't be issues on any of the CI runs (not sure what some of the Windows paths could look like, so these filters might need to be changed):Test Plan
crates/uv/tests/venv.rs
and updated snapshots.uv venv
in a Docker container to test that the output is correct (running Ubuntu)