Added pyvm use <version> for session-level switching#64
Added pyvm use <version> for session-level switching#64shreyasmene06 merged 10 commits intoshreyasmene06:mainfrom
Conversation
|
@shreyasmene06 kindly verify the pr |
There was a problem hiding this comment.
Thanks for tackling this feature! The subshell approach is a sensible way to implement session-level switching. However, there are several issues that need to be fixed before merging.
Required Changes
1. Wrong function: click.exit() should be sys.exit()
Throughout the code you're using click.exit(1) which doesn't exist. Click uses sys.exit() or you can raise SystemExit(1):
# ❌ Wrong (lines 625, 634, 680, 705)
click.exit(1)
# ✅ Correct
sys.exit(1)This will cause AttributeError: module 'click' has no attribute 'exit' at runtime.
2. Inconsistent indentation
Several lines have incorrect indentation (extra space):
Lines 624-625, 628-638, 659-667, 676, 695-699
if not validate_version_string(version):
click.echo(...) # <-- 5 spaces instead of 4
click.exit(1)
Please fix to use consistent 4-space indentation throughout.
3. Missing tests
This is a significant new feature that modifies shell behavior. Please add tests for:
- Version validation
- Finding Python executable
- Symlink creation logic
- Error handling (version not found, symlink failures)
Even if you can't easily test the subshell spawning, the setup/teardown logic can be unit tested.
Suggestions (non-blocking)
-
Security: validate version before using in paths
The version string is used directly in tempfile.mkdtemp(prefix=f"pyvm_session_{version}_"). While mkdtemp is safe, consider validating the version more strictly to prevent any path traversal edge cases.
-
Windows pip linking is inside the else block
The pip symlink logic (lines 672-678) is inside the else: (non-Windows) branch, so pip won't be linked on Windows.
-
Consider using os.execvpe instead of subprocess.run
Using subprocess.run([current_shell]) creates a child process. Using os.execvpe would replace the current process, which is more typical for shell switching tools. However, your approach works and allows for cleanup, so this is optional.
-
Add --shell option?
Users might want to specify a different shell than their default.
|
@shreyasmene06 i have made all the fixes kindly verify the files now |
|
@shreyasmene06 kindly merge the PR |
shreyasmene06
left a comment
There was a problem hiding this comment.
All previous feedback has been addressed:
- Fixed
click.exit()→sys.exit()throughout - Fixed inconsistent indentation
- Added comprehensive tests (test_cli_use.py)
- Fixed Windows pip linking (now checks Scripts directory)
- The implementation is clean:
- Subshell approach is sensible for session-level switching
- Proper cleanup with
shutil.rmtreeinfinallyblock - Good fallback to
.batshim when Windows symlinks fail - Tests cover Linux, Windows, invalid version, and version-not-found cases
One minor note (non-blocking): The commented-out assertion intest_use_version_success_linux(line 39-40) could be uncommented or removed to keep tests clean, but that's trivial.
LGTM! 🎉
|
@ParthG2209 everything is perfectly done just make sure the formatting checks are passed you can see the contribution.md there I have shared how to run the tests locally |
|
@shreyasmene06 i have made the changes, kindly add the acwoc and difficulty tags and merge the PR |
|
@ParthG2209 I can see still the formatting checks have failed please make sure these pass |
|
@shreyasmene06 kindly verify the files now |
|
@shreyasmene06 pls verify the PR |
|
@shreyasmene06 kindly verify the PR |
shreyasmene06
left a comment
There was a problem hiding this comment.
Everything is perfect but you just need to make sure the workflow check passes as these small things might cause a big vulnerability later...
@shreyasmene06 All tests have been cleared kindly merge the PR |
fixes #54
I added a new use command to the CLI that lets you temporarily switch Python versions for your current session. It works by creating a temporary environment with symlinks to the requested version and spawning a subshell, so when you exit, you are right back to your default setup. This makes testing across different versions much easier without changing global defaults.