-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add ability to customize pyenv-win-venv installation location #35
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis PR adds functionality to customize the installation location of pyenv-win-venv instead of using the default location at %USERPROFILE%.pyenv-win-venv. The implementation modifies the installation script to prompt users for their preferred installation location and updates all related scripts to use the custom location instead of the hardcoded default path. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @jacquindev - I've reviewed your changes - here's some feedback:
Overall Comments:
- The batch files (pyenv-win-venv.bat and pyenv-venv.bat) are incomplete - they set ScriptDir but don't use it to launch the PowerShell scripts. This will break Windows compatibility.
- Consider replacing the interactive prompt with a command-line parameter for the installation path to support automated installations and CI/CD scenarios.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
$PyEnvWinVenvDir = "${env:USERPROFILE}\.pyenv-win-venv" | ||
} | ||
else { | ||
$PyEnvWinVenvDir = Read-Host "Input your desired location for pyenv-win-venv installation" |
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.
issue: The custom installation path should be validated before proceeding
Consider adding path validation to ensure the provided location is valid, writable, and doesn't contain invalid characters. This will prevent installation failures later in the script.
Instead of using the default location at
$env:USERPROFILE\.pyenv-win-venv
, we can now use pyenv-win-venv from different location. For example: D:\tools\pyenv-win-venv.Summary by Sourcery
Introduce the ability to customize the installation location of pyenv-win-venv, providing users with the flexibility to choose a directory other than the default. Enhance the script by improving error handling during the removal of installation scripts.
New Features:
Enhancements: