fix: respect local config file in poetry python commands#10596
fix: respect local config file in poetry python commands#10596finswimmer wants to merge 2 commits intopython-poetry:mainfrom
poetry python commands#10596Conversation
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.
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.
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.
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: