Skip to content
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: update, add and remove with --only shall not uninstall dependencies from all other groups #10014

Merged
merged 1 commit into from
Jan 11, 2025

Conversation

radoering
Copy link
Member

@radoering radoering commented Jan 10, 2025

Reintroduce some logic removed in bd3500d and add tests for it. Fix logic with reresolve = False, which had the same issue.

  • The change in installer is the fix for reresolve = True (reintroducing old logic).
  • The change in transaction is the fix for reresolve = False.

Pull Request Check List

Resolves: #9950

(Does not resolve the similar issue #9975. This one requires another fix.)

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

Summary by Sourcery

Restore the logic for the --only flag to prevent uninstalling dependencies from other groups, and add tests to cover this behavior.

Bug Fixes:

  • Fix an issue where using the --only flag with update, add, and remove commands would uninstall dependencies from all other groups.

Tests:

  • Add tests to verify the fix for the --only flag behavior.

…ependencies from all other groups

Reintroduce some logic removed in bd3500d and add tests for it.
Fix logic with `reresolve = False`, which had the same issue.

- The change in installer is the fix for `reresolve = True` (reintroducing old logic).
- The change in transaction is the fix for `reresolve = False`.
Copy link

sourcery-ai bot commented Jan 10, 2025

Reviewer's Guide by Sourcery

This pull request fixes a bug where using the --only option with update, add, or remove commands would uninstall dependencies from groups that were not specified. The fix reintroduces logic that was previously removed and adds tests to cover the corrected behavior. Two distinct scenarios are addressed: one for when reresolve=True and another for when reresolve=False.

Sequence diagram for package installation with --only flag

sequenceDiagram
    participant User
    participant Installer
    participant Transaction
    participant Repository

    User->>Installer: Install with --only flag
    Installer->>Transaction: calculate_operations(with_uninstalls)
    alt reresolve=True
        Transaction->>Repository: Get locked packages
        Transaction->>Repository: Get lockfile packages
        Transaction->>Transaction: Calculate uninstall ops
        Transaction->>Transaction: Calculate install ops
        Transaction-->>Installer: Return combined ops
    else reresolve=False
        Transaction->>Transaction: Check result packages
        Transaction->>Transaction: Calculate ops for specified groups
        Transaction-->>Installer: Return filtered ops
    end
    Installer-->>User: Execute operations
Loading

Class diagram for Transaction and Installer changes

classDiagram
    class Transaction {
        -_result_packages: List
        -_current_packages: List
        -_installed_packages: List
        +calculate_operations(with_uninstalls: bool): List
    }

    class Installer {
        -_requires_synchronization: bool
        -_update: bool
        -_extras: List
        +_do_install(): int
    }

    class Operation {
        +job_type: str
    }

    Transaction ..> Operation
    Installer ..> Transaction

    note for Transaction "Modified uninstall logic to respect --only flag"
    note for Installer "Updated operation calculation based on reresolve flag"
Loading

Flow diagram for dependency resolution logic

flowchart TD
    A[Start Installation] --> B{Reresolve?}
    B -->|Yes| C[Calculate operations]
    C --> D[Get uninstall operations]
    D --> E[Combine with install operations]
    B -->|No| F[Calculate operations with filtered uninstalls]
    E --> G[Execute operations]
    F --> G
    G --> H[End Installation]

    subgraph Uninstall Logic
    D
    F
    end
Loading

File-Level Changes

Change Details Files
Reintroduced logic to prevent uninstalling dependencies from unspecified groups when reresolve=True. This fix addresses the issue where using --only with update, add, or remove would unintentionally remove packages not belonging to the specified groups.
  • Added conditional check around self._requires_synchronization or self._update to determine when to calculate uninstall operations.
  • Included not reresolve in the conditional check to prevent unnecessary uninstalls when reresolve is True and synchronization is not required.
src/poetry/installation/installer.py
Fixed the logic for handling uninstallations when reresolve=False to prevent unintended removal of packages from unspecified groups.
  • Modified the condition for uninstalling packages to include a check for result_packages to ensure only packages not present in the resolved dependencies are uninstalled.
src/poetry/puzzle/transaction.py
Added tests to verify the fix for both reresolve=True and reresolve=False scenarios.
  • Added parameters with_uninstalls and sync to test different scenarios.
  • Included assertions to validate the expected behavior of the calculate_operations method.
  • Added test cases for dependency groups and markers.
  • Parameterized tests to cover different lock versions, update strategies, and synchronization requirements.
tests/puzzle/test_transaction.py
tests/installation/test_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.
  • 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

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 @radoering - I've reviewed your changes and they look great!

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

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.

tests/puzzle/test_transaction.py Show resolved Hide resolved
@abn abn merged commit 30fe6c7 into python-poetry:main Jan 11, 2025
73 checks passed
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.

Unexpected breaking change with --only option?
2 participants