poetry icon indicating copy to clipboard operation
poetry copied to clipboard

fix(application): handle global options correctly

Open abn opened this issue 11 months ago • 4 comments

Previously, the application implementation did not handle global options correctly when positioned after command options. This lead to incorrect cached working directory values prior to switching application context.

Resolves: #9978

Summary by Sourcery

Bug Fixes:

  • Fixed an issue where global options were not handled correctly when positioned after command options, leading to incorrect cached working directory values.

abn avatar Jan 11 '25 23:01 abn

Reviewer's Guide by Sourcery

This pull request fixes a bug where global options were not handled correctly when positioned after command options, leading to incorrect cached working directory values. The fix involves refactoring the Application class to correctly handle global options such as --directory and --project. Several tests were added to cover the different scenarios and ensure the fix is effective.

Sequence diagram for global options handling in Application

sequenceDiagram
    participant App as Application
    participant IO as IO
    participant Dir as Directory Utils

    App->>App: _run(io)
    App->>App: _configure_custom_application_options(io)
    App->>IO: Get option values (directory, project)
    App->>Dir: ensure_path(directory)
    Note right of Dir: Validate directory path
    Dir-->>App: Return validated working_directory
    App->>Dir: ensure_path(project)
    Note right of Dir: Validate project path
    Dir-->>App: Return validated project_directory

Class diagram showing Application class changes

classDiagram
    class Application {
        -_working_directory: Path
        -_project_directory: Path
        -_disable_plugins: bool
        -_disable_cache: bool
        +_run(io: IO): int
        +_configure_custom_application_options(io: IO): void
        +_option_get_value(io: IO, name: str, default: Any): Any
    }

    class Helpers {
        +ensure_path(path: str|Path, is_directory: bool): Path
    }

    Application ..> Helpers: uses

Flow diagram for global options processing

flowchart TD
    A[Start] --> B[Get IO input]
    B --> C{Has directory option?}
    C -->|Yes| D[Validate directory path]
    C -->|No| E[Use current directory]
    D --> F{Has project option?}
    E --> F
    F -->|Yes| G[Validate project path]
    F -->|No| H[Use working directory]
    G --> I[Set working and project directories]
    H --> I
    I --> J[End]

File-Level Changes

Change Details Files
Refactored the Application class to correctly handle global options (--directory, --project).
  • Removed cached properties for _project_directory and _working_directory.
  • Added a new method _configure_custom_application_options to handle global options.
  • Added a helper method _option_get_value to retrieve option values.
  • Used ensure_path to validate directory paths.
  • Modified _run to call _configure_custom_application_options before loading plugins.
  • Corrected the logic for determining the project and working directories based on the global options.
src/poetry/console/application.py
Added tests to verify the fix and cover different scenarios.
  • Added tests for various global option positions.
  • Added tests to ensure correct behavior with valid and invalid project/directory paths.
  • Added tests to verify correct project and working directory resolution with relative and absolute paths.
tests/console/test_application_global_options.py
Removed unused imports and code related to the old implementation.
  • Removed unused imports in test_version.py.
  • Removed tests that were no longer relevant.
  • Added ensure_path function to helpers.py.
  • Added tests for ensure_path function in test_helpers.py.
tests/console/commands/test_version.py
tests/utils/test_helpers.py
src/poetry/utils/helpers.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.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull request title to generate a 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. You can also use this command to specify where the summary should be inserted.

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

  • Contact our support team for questions or feedback.
  • Visit our documentation for detailed guides and information.
  • Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai[bot] avatar Jan 11 '25 23:01 sourcery-ai[bot]

The issue reported in #9978 can be reproduced and the fix in this PR tested using the following snippet.

podman run --rm -i --entrypoint bash python:latest <<EOF
set -x
python -m pip install --disable-pip-version-check --root-user-action ignore -q poetry
pushd /tmp
poetry new foobar
poetry add -C /tmp/foobar httpx
poetry show --only main -C /tmp/foobar || :
python -m pip install --disable-pip-version-check --root-user-action ignore -q --force 'git+https://github.com/python-poetry/poetry.git@refs/pull/10021/head'
poetry show --only main -C /tmp/foobar
EOF

abn avatar Jan 11 '25 23:01 abn

@sourcery-ai review

abn avatar Jan 12 '25 19:01 abn

Changed the path resolution for --project to avoid doing a hard cwd switch.

abn avatar Jan 12 '25 20:01 abn

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

github-actions[bot] avatar Feb 14 '25 00:02 github-actions[bot]