Skip to content

Conversation

@finswimmer
Copy link
Member

@finswimmer finswimmer commented Oct 24, 2025

#10595 should be merged first, otherwise we are unable to set the relevant configs anyway.

Pull Request Check List

Resolves: #issue-number-here

  • Added tests for changed code.
  • Updated documentation for changed code.

Summary by Sourcery

Enhancements:

  • Add PoetryPythonPathProvider.base_installation_dir to load and merge local poetry.toml into the config for determining python installation paths

@sourcery-ai
Copy link

sourcery-ai bot commented Oct 24, 2025

Reviewer's Guide

This 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 usage

classDiagram
    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
Loading

Flow diagram for resolving python installation directory with local config

flowchart 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"]
Loading

File-Level Changes

Change Details Files
Added base_installation_dir method that merges local poetry.toml into the global config
  • Implemented classmethod in PoetryPythonPathProvider to locate and read local poetry.toml
  • Merged local config into default Config object under a suppress context
  • Exposed merged python_installation_dir as base_installation_dir
src/poetry/utils/env/python/providers.py
Updated provider methods to use the new base_installation_dir
  • Changed installation_dir to call base_installation_dir
  • Updated _make_bin_paths to fall back on base_installation_dir
src/poetry/utils/env/python/providers.py
Replaced direct Config.create().python_installation_dir calls in CLI commands
  • Removed Config import and used PoetryPythonPathProvider.installation_dir in remove command
  • Switched list command to use PoetryPythonPathProvider.base_installation_dir
src/poetry/console/commands/python/remove.py
src/poetry/console/commands/python/list.py
Updated PythonInstaller to use the new provider for default installation directory
  • Removed direct Config.create call from default_factory
  • Switched to PoetryPythonPathProvider.base_installation_dir in installer constructor
src/poetry/utils/env/python/installer.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@finswimmer finswimmer requested a review from a team October 24, 2025 09:51
@finswimmer finswimmer force-pushed the python-installation-dir branch from cb5c07d to 4f71bfd Compare October 24, 2025 18:44
@finswimmer finswimmer marked this pull request as ready for review October 24, 2025 18:45
Copy link

@sourcery-ai sourcery-ai bot left a 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 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.
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.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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()
Copy link
Member

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.

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants