-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: respect local config file in poetry python commands
#10596
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR refactors the determination of Poetry’s python installation directory by introducing a new base_installation_dir method that merges settings from a local poetry.toml, and updates all provider implementations and console commands to use this method instead of directly calling Config.create(). Class diagram for updated PoetryPythonPathProvider and related usageclassDiagram
class PoetryPythonPathProvider {
+base_installation_dir() Path
+installation_dir(version: Version, implementation: str) Path
+_make_bin_paths(base: Path | None) list[Path]
}
class Config {
+create() Config
python_installation_dir Path
+merge(data)
}
class TOMLFile {
+read()
+exists()
}
class Factory {
+locate()
}
class PythonInstaller
class python_remove_py
class python_list_py
PoetryPythonPathProvider --|> PathProvider
PoetryPythonPathProvider ..> Config : uses
PoetryPythonPathProvider ..> TOMLFile : uses
PoetryPythonPathProvider ..> Factory : uses
PythonInstaller o-- PoetryPythonPathProvider : uses base_installation_dir
python_remove_py o-- PoetryPythonPathProvider : uses installation_dir
python_list_py o-- PoetryPythonPathProvider : uses base_installation_dir
Flow diagram for resolving python installation directory with local configflowchart TD
A["Start: Need python installation directory"] --> B["Check for local poetry.toml config file"]
B -->|Exists| C["Merge local config into main config"]
B -->|Does not exist| D["Use main config only"]
C --> E["Return python_installation_dir from merged config"]
D --> E
E["Directory used for python installation"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
cb5c07d to
4f71bfd
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.
Hey there - I've reviewed your changes - here's some feedback:
- Add targeted tests to verify that each
poetry pythoncommand actually picks up and respects overrides in a localpoetry.tomlfile. - Since
base_installation_dirre-reads and merges the config on every call, consider caching the merged result to avoid repetitive file I/O and parsing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add targeted tests to verify that each `poetry python` command actually picks up and respects overrides in a local `poetry.toml` file.
- Since `base_installation_dir` re-reads and merges the config on every call, consider caching the merged result to avoid repetitive file I/O and parsing.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| # Let's check if there is a local config file. | ||
| with contextlib.suppress(RuntimeError): | ||
| pyproject = Factory.locate() |
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, we have to pass the project directory to base_installation_dir() and call Factory.locate() with it. Otherwise, --project and --directory will not work. In Command classes, we can call self.get_application().project_directory. Then, we will probably have to pass it to all relevant methods.
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.
Good catch 👍 I forgot these command line arguments. Looks like it is not trivial to pass it to all relevant methods 🤔 I will have a closer look at it the next days.
#10595 should be merged first, otherwise we are unable to set the relevant configs anyway.
Pull Request Check List
Resolves: #issue-number-here
Summary by Sourcery
Enhancements: