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

installer: remove unrequested extras if requires_synchronization is set #8621

Merged

Conversation

radoering
Copy link
Member

@radoering radoering commented Nov 4, 2023

tl;dr

  • fix an issue where poetry install --sync does not remove unrequested extras (funny enough poetry install without --sync does remove them)
  • increase test coverage of the installer regarding extras
  • Added tests for changed code.
  • Updated documentation for changed code.

Long story

Originally, I was just poking around with python-poetry/poetry-core#613 (comment). However, I noticed that all installer tests still passed if one of the following lines were removed:

Thus, I decided to improve the test coverage before trying to fix bugs. I merged some quite similar tests and used pytest.mark.parametrize to cover the missing cases. However,

if locked.optional and locked.name not in extra_packages:
# Installed but optional and not requested in extras
ops.append(Uninstall(locked))
was still irrelevant.

After some code spelunking, I suppose that these lines became irrelevant with ea8fb8c. However, that introduced the mentioned bug. After adding @pytest.mark.parametrize("do_sync", [False, True]) to the test and fixing the bug, the mentioned lines of code are relevant again.

@radoering radoering force-pushed the installer-extras-test-coverage branch from 0aef22d to a8d2002 Compare November 10, 2023 16:33
@radoering radoering requested a review from a team November 10, 2023 16:39
@radoering radoering force-pushed the installer-extras-test-coverage branch 3 times, most recently from d6d3895 to cd4f7b3 Compare November 19, 2023 16:58
Copy link
Member

@Secrus Secrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

… set, increase test coverage of installer regarding extras
@radoering radoering force-pushed the installer-extras-test-coverage branch from cd4f7b3 to 9eb5363 Compare November 21, 2023 15:33
@radoering radoering merged commit e6c1c99 into python-poetry:master Nov 21, 2023
20 checks passed
MrGreenTea pushed a commit to MrGreenTea/poetry that referenced this pull request Dec 18, 2023
… set (python-poetry#8621)

... and increase test coverage of installer regarding extras
Copy link

github-actions bot commented Mar 3, 2024

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 github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants