Skip to content

Commit

Permalink
Executor: Remove duplicate entry with dry-run argument (#4660)
Browse files Browse the repository at this point in the history
# Pull Request Check List

Resolves: #3097 

<!-- This is just a reminder about the most common mistakes. Please make sure that you tick all *appropriate* boxes.  But please read our [contribution guide](https://python-poetry.org/docs/contributing/) at least once, it will save you unnecessary review cycles! -->

- [x] Added **tests** for changed code. I adapted the test in the `tests/console/commands/plugin/test_remove.py` file to reflect the bug fix.
- [ ] Updated **documentation** for changed code.

<!-- If you have *any* questions to *any* of the points above, just **submit and ask**!  This checklist is here to *help* you, not to deter you from contributing! -->

Apart from running the ci linting steps and pytest locally for both python 3.6 and 3.8, I also tested the code by running
```
docker run --rm -i --entrypoint bash python:3.8 <<EOF
set -e
python -m pip install -q git+https://github.com/mmacchia/poetry.git@issue/3097
python -m poetry new foobar
pushd foobar
sed -i /pytest/d pyproject.toml
python -m poetry add --dry-run pycowsay
python -m poetry run pip list
EOF
```
The corresponding output is:
```
Created package foobar in foobar
/foobar /
Creating virtualenv foobar-lWDpn5M1-py3.8 in /root/.cache/pypoetry/virtualenvs
Using version ^0.0.0.1 for pycowsay

Updating dependencies
Resolving dependencies...

Writing lock file

Package operations: 1 install, 0 updates, 0 removals

  • Installing pycowsay (0.0.0.1)
Package    Version
---------- -------
pip        21.2.4
setuptools 58.1.0
wheel      0.37.0
```
As expected, the command `poetry add --dry-run pycowsay` yielded a single output (` • Installing pycowsay (0.0.0.1)`) and did not add the package to the project (see the output of `poetry run pip list`).

The change in the PR solves the duplication issue because the creation of the console output when `not self._enabled` is taken care of by the boolean output of the`self._should_write_operation(operation)` function.

Please feel free to get in touch if I can help adding more test cases for the `--dry-run` argument or the `_enabled` flag and to add any feedback. I would be happy to improve/clarify the code in this PR.

Co-authored-by: Marco Macchia <mmacchia@wayfair.com>
  • Loading branch information
mmacchia and Marco Macchia committed Aug 24, 2022
1 parent 92033c7 commit 75cf77b
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 4 deletions.
6 changes: 3 additions & 3 deletions src/poetry/installation/executor.py
Expand Up @@ -316,8 +316,6 @@ def _do_execute_operation(self, operation: Operation) -> int:
return 0

if not self._enabled or self._dry_run:
self._io.write_line(f" <fg=blue;options=bold>•</> {operation_message}")

return 0

result: int = getattr(self, f"_execute_{method}")(operation)
Expand Down Expand Up @@ -723,7 +721,9 @@ def _download_archive(self, operation: Install | Update, link: Link) -> Path:
return archive

def _should_write_operation(self, operation: Operation) -> bool:
return not operation.skipped or self._dry_run or self._verbose
return (
not operation.skipped or self._dry_run or self._verbose or not self._enabled
)

def _save_url_reference(self, operation: Operation) -> None:
"""
Expand Down
1 change: 0 additions & 1 deletion tests/console/commands/self/test_remove_plugins.py
Expand Up @@ -95,7 +95,6 @@ def test_remove_installed_package_dry_run(tester: CommandTester):
Package operations: 0 installs, 0 updates, 1 removal, 1 skipped
• Removing poetry-plugin (1.2.3)
• Removing poetry-plugin (1.2.3)
• Installing poetry ({__version__}): Skipped for the following reason: Already \
installed
Expand Down

0 comments on commit 75cf77b

Please sign in to comment.