-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix "env use python" to use Python in PATH #10187
Conversation
Reviewer's Guide by SourceryThis pull request modifies the way Poetry locates Python executables. It now uses Sequence diagram for locating Python executablesequenceDiagram
participant User
participant Poetry
participant ShutilWhichPythonProvider
participant findpython
User->>Poetry: env use python
Poetry->>ShutilWhichPythonProvider: find_python_by_name("python")
ShutilWhichPythonProvider->>shutil: which("python")
alt Python found in PATH
shutil-->>ShutilWhichPythonProvider: Path to python
ShutilWhichPythonProvider->>findpython: PythonVersion(executable=Path(path))
findpython-->>ShutilWhichPythonProvider: PythonVersion object
ShutilWhichPythonProvider-->>Poetry: PythonVersion object
Poetry->>Poetry: Use PythonVersion object to create Python environment
else Python not found in PATH
shutil-->>ShutilWhichPythonProvider: None
ShutilWhichPythonProvider-->>Poetry: None
Poetry->>findpython: find(python_name)
end
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 @radoering - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a test case where
python_name
inget_by_name
is an absolute path to a python executable.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 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.
# if it is a path try assuming it is an executable | ||
return cls.from_executable(python_name) | ||
if python := ShutilWhichPythonProvider.find_python_by_name(python_name): | ||
return cls(python=python) | ||
|
||
if python := findpython.find(python_name): |
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.
Not a blocker for this, but I am definitely curious if the reason why findpython.find(python_name)
does not work as expected here has to do with how the action sets up the Python installation.
This should use the Python, which is in the PATH, not the latest Python.
d74ea36
to
9d487ca
Compare
This should use the Python, which is in the PATH, not the latest Python.
Pull Request Check List
Resolves: #10186
Summary by Sourcery
Fix the
env use python
command to select the Python executable specified in the user's PATH.Bug Fixes:
env use python
instead of the latest Python version available.Tests:
env use python
.