Add --ask option to pip-sync #913

georgek · 2019-09-27T10:14:35Z

This adds an --ask option to pip-sync which will first show the user what would change, like --dry-run, and then ask to confirm before continuing with the changes. This is supposed to address issue #671 without breaking existing workflows (in particular CI pipelines) and in a way that is well-established by existing tools like nmcli (network manager) and portage (Gentoo package manager).

This change requires a minor version bump.

Changelog-friendly one-liner: Add --ask option to pip-sync.

Contributor checklist

Provided the tests for the changes.
Requested a review from another contributor.
Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

georgek · 2019-09-27T10:15:19Z

@atugushev Please could you review?

codecov · 2019-09-27T13:38:57Z

Codecov Report

Merging #913 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #913      +/-   ##
=========================================
+ Coverage   98.58%   98.6%   +0.02%     
=========================================
  Files          35      35              
  Lines        2264    2300      +36     
  Branches      290     298       +8     
=========================================
+ Hits         2232    2268      +36     
  Misses         21      21              
  Partials       11      11

Impacted Files	Coverage Δ
piptools/scripts/sync.py	`100% <100%> (ø)`	⬆️
tests/test_sync.py	`100% <100%> (ø)`	⬆️
tests/test_cli_sync.py	`100% <100%> (ø)`	⬆️
piptools/sync.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd8aee7...1269d78. Read the comment docs.

atugushev

Thanks, @georgek! I like the way you've refactored sync. A few comments. I think we shouldn't change dry_run value during runtime, but check ask explicitly instead. Here are the changes I propose:

piptools/sync.py

tests/test_sync.py

atugushev · 2019-09-27T15:53:20Z

tests/test_sync.py

@@ -437,6 +437,64 @@ def test_sync_dry_run_would_uninstall(echo, from_line):
    echo.assert_has_calls(expected_calls, any_order=True)


+@mock.patch("piptools.sync.click.confirm")
+@mock.patch("piptools.sync.click.echo")
+def test_sync_ask_declined_would_install(echo, confirm, from_line):


Tests test_sync_ask_declined_would_install and test_sync_ask_declined_would_uninstall looks almost the same. Could we use parametrization instead?

It can be done but it's a bit awkward. Since I tried to mirror the tests test_sync_dry_run_would_uninstall and test_sync_dry_run_would_install I have also parameterised these as well.

tests/test_sync.py

tests/test_cli_sync.py

tests/test_sync.py

atugushev

Almost done! Great work 👍

atugushev

LGTM 👍

georgek · 2019-09-30T18:06:23Z

Thanks @atugushev!

Please let me know when I should squash/rebase etc.

atugushev · 2019-10-01T22:41:07Z

@georgek

Please let me know when I should squash/rebase etc.

Yes, please, squash and rebase it.

atugushev · 2019-10-04T04:24:45Z

Thanks, @georgek! Awesome work! 👍

georgek force-pushed the feature/pip-sync-ask-option branch from 181baea to 90b8a3b Compare September 27, 2019 12:36

atugushev reviewed Sep 27, 2019

View reviewed changes

atugushev added the enhancement Improvements to functionality label Sep 27, 2019

atugushev added this to the 4.2.0 milestone Sep 27, 2019