feat: add dry-run option #78

max-wittig · 2018-10-02T19:14:15Z

No description provided.

peterbe · 2018-10-02T19:22:13Z

I restarted the failing (3.6) tests.

peterbe

This is great! I'm tempting to land it as is but there's one major thing that I don't like and that it doesn't actually test what changes would be made.

See:

dev/PYTHON/hashin  (63a92a9...) ✔                                                                            
▶ touch /tmp/reqs.txt
dev/PYTHON/hashin  (63a92a9...) ✔                                                                            
▶ python hashin.py -r /tmp/reqs.txt requests
dev/PYTHON/hashin  (63a92a9...) ✔                                                                            
▶ python hashin.py -r /tmp/reqs.txt --dry-run requests
requests==2.19.1 \
    --hash=sha256:63b52e3c866428a224f97cab011de738c36aec0185aa91cfacd418b5d58911d1 \
    --hash=sha256:ec22d826a36ed72a7358ff3fe56cbd4ba69dd7a6718ffd450ff0e9df7a47ce6a

What it actually should notice is that the version of requests hasn't actually changed in /tmp/reqs.txt.

README.rst

peterbe · 2018-10-03T17:27:46Z

So, your patch is this:

    if dry_run:
        if verbose:
            _verbose('Dry run, not editing ', file)
        print(new_lines)
        return
    if verbose:
        _verbose('Editing', file)
    with open(file) as f:
        requirements = f.read()
    requirements = amend_requirements_content(
        requirements,
        package,
        new_lines
    )
    with open(file, 'w') as f:
        f.write(requirements)

Perhaps what we could do is something like this:

    if verbose:
        _verbose('Editing', file)
    with open(file) as f:
        requirements = f.read()
    requirements = amend_requirements_content(
        requirements,
        package,
        new_lines
    )
    if dry_run:
        if verbose:
            _verbose('Dry run, not editing ', file)
        with open(file) as f:
            old_requirements = f.read()
        print(diff(old_requirements, requirements))
    else:
        with open(file, 'w') as f:
            f.write(requirements)

What do you think?

max-wittig · 2018-10-04T05:55:39Z

@peterbe Thanks for the review. Will fix it probably on the weekend

max-wittig · 2018-10-05T16:18:14Z

@peterbe Actually I'm not sure about the diff. It would remove my use-case for this function.

Normally I use pipenv for handling lock files, hashes etc.
But to install pipenv in a CI environment, I need to use pip with hashes.
Using hashin directly is not possible, because hashin doesn't actually resolve all dependencies of a requirements, so when I run hashin pipenv I only get the hashes for pipenv itself.
But actually I also want the hashes for all dependencies of pipenv. To accomplish that I usually:

Install everything with pip in a virtualenv
Freeze all packages
Run this cat requirements.txt | while read line; do hashin -v $line; done
Rename the requirements to requirements-pipenv.txt
Run pip install --require-hashes -r requirements-pipenv.txt
Use pipenv for all other packages e.g. pipenv install requests

With the dry-run feature I could quickly copy&paste everything into the requirements-pipenv.txt file and be done with it.

Maybe dry-run is simply the wrong name for this and I need to open a separate PR after this one. What do you think?

max-wittig · 2018-10-05T16:32:42Z

@peterbe Thinking more about it, I've applied your diff approach now.

Maybe I will open a separate PR with another argument later (e.g. --raw)

mythmon · 2018-10-05T17:05:10Z

@max-wittig I don't totally follow what you're trying to do, but have you seen the -r option for hashin? It changes the file that hashin writes to. Would that help your current workflow?

max-wittig · 2018-10-05T17:06:28Z

@peterbe I will look into it. I've made your requested changes for this PR. I think some people will still find it useful 😄

peterbe

Thanks for being understand and that you're not giving up on this PR even if it might not be what you're after.

peterbe · 2018-10-05T17:37:32Z

README.rst

+
+    @@ -3,3 +3,6 @@
+
+         --hash=sha256:ad2edb7da2fc50a1d1a117cc9cfd8fc1f8b30c8b04932d5efef4fb09cbb2e984


There's too many empty newlines in this diff output. Also, see my comment below about unified_diff.

peterbe · 2018-10-05T17:38:00Z

tests/test_cli.py

+
+        # Check dry run output
+        out_lines = my_stdout.getvalue().splitlines()
+        print(out_lines)


Debug leftover.

peterbe · 2018-10-05T18:03:29Z

tests/test_arg_parse.py

@@ -53,5 +56,6 @@ def test_minimal():
        verbose=False,
        version=False,
        include_prereleases=False,
+        dry_run=False,


Isn't this the default anyway?

Tests fail otherwise here

Should I leave it like this or what changes should be made?

tests/test_arg_parse.py

peterbe · 2018-10-05T18:05:59Z

hashin.py

@@ -175,17 +182,25 @@ def run_single_package(
            new_lines += ' \\'
        new_lines += '\n'

-    if verbose:
-        _verbose('Editing', file)
    with open(file) as f:
        requirements = f.read()


Change this to old_requirements = f.read() and 2 lines down pass in old_requirements (instead of requirements) to the amend_requirements_content function.
Then you don't need lines 195-196 since you'll already have a copy of the whole old file content.

peterbe · 2018-10-05T18:10:44Z

hashin.py

+        with open(file) as f:
+            old_requirements = f.read()
+        for line in difflib.unified_diff(old_requirements.splitlines(), requirements.splitlines()):
+            print(line)


The correct way to print a nice diff is to do it like this:

old = """ One Two Three """.strip() new = """ One One and a half Twoish Three """.strip() print( "".join( difflib.unified_diff( old.splitlines(True), new.splitlines(True), fromfile="Old", tofile="New" ) ) )

Then you get an output which resembles what you get with git diff kinda:

--- Old +++ New @@ -1,3 +1,4 @@ One -Two +One and a half +Twoish Three

This snippet works in Python 2 and Python 3.

Thanks for all this help in fixing the MR!

peterbe

Did you want to take a look @mythmon ?

mythmon

I have a couple nits, but overall this looks good to me.

mythmon · 2018-10-05T19:41:53Z

README.rst

+
+There are some use cases, when you maybe don't want to edit your ``requirements.txt``
+right away. You can use the ``-dry-run`` argument to just print the lines, as if they
+would be added to your requirements.txt file.


I think that this still should have backticks aroud requirements.txt. Also, the argument is listed as -dry-run with only one -, when it should two.

More holistically, didn't this change so that it shows a diff, isntead of "just print[ing] the lines"?

mythmon · 2018-10-05T19:42:31Z

hashin.py

@@ -75,6 +76,12 @@
    action='store_true',
    default=False,
 )
+parser.add_argument(
+    '--dry-run',
+    help='Don\'t touch requirements.txt and just show hashes',


Nit: Most style guides allows for switching quotes to avoid escaping quotes in a string.

I wouldn't worry about this one. Once this PR is squared away there are no open PRs and I can take a massive knife of black and therapist to this code base and then these kinds of nits become unimportant.

peterbe · 2018-10-08T12:19:07Z

hashin 0.13.4 just released.

max-wittig added 3 commits October 2, 2018 21:07

feat: add dry-run option

fe8fa29

Fixes peterbe#30

docs(readme): add dry-run documentation

9ddd350

chore(tests): add dry-run tests

63a92a9

peterbe requested changes Oct 3, 2018

View reviewed changes

README.rst Outdated Show resolved Hide resolved

peterbe requested changes Oct 5, 2018

View reviewed changes

max-wittig force-pushed the feat/dry-run branch from e94d3c9 to 9028fe5 Compare October 5, 2018 19:22

peterbe approved these changes Oct 5, 2018

View reviewed changes

mythmon reviewed Oct 5, 2018

View reviewed changes

max-wittig force-pushed the feat/dry-run branch from 9028fe5 to ce4ab79 Compare October 5, 2018 19:52

fix(dry-run): show diff with dry-run option

db4da3c

max-wittig force-pushed the feat/dry-run branch from ce4ab79 to db4da3c Compare October 5, 2018 19:54

peterbe merged commit 32445d8 into peterbe:master Oct 8, 2018

max-wittig deleted the feat/dry-run branch October 8, 2018 14:31

max-wittig mentioned this pull request Oct 18, 2018

Add a dry-run option #30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add dry-run option #78

feat: add dry-run option #78

max-wittig commented Oct 2, 2018

peterbe commented Oct 2, 2018

peterbe left a comment

peterbe commented Oct 3, 2018

max-wittig commented Oct 4, 2018 •

edited

Loading

max-wittig commented Oct 5, 2018

max-wittig commented Oct 5, 2018

mythmon commented Oct 5, 2018

max-wittig commented Oct 5, 2018

peterbe left a comment

peterbe Oct 5, 2018

peterbe Oct 5, 2018

peterbe Oct 5, 2018

max-wittig Oct 5, 2018

max-wittig Oct 5, 2018

peterbe Oct 5, 2018

peterbe Oct 5, 2018

max-wittig Oct 5, 2018

peterbe left a comment

mythmon left a comment

mythmon Oct 5, 2018

mythmon Oct 5, 2018

peterbe Oct 5, 2018

peterbe commented Oct 8, 2018


		@@ -3,3 +3,6 @@

		--hash=sha256:ad2edb7da2fc50a1d1a117cc9cfd8fc1f8b30c8b04932d5efef4fb09cbb2e984

feat: add dry-run option #78

feat: add dry-run option #78

Conversation

max-wittig commented Oct 2, 2018

peterbe commented Oct 2, 2018

peterbe left a comment

Choose a reason for hiding this comment

peterbe commented Oct 3, 2018

max-wittig commented Oct 4, 2018 • edited Loading

max-wittig commented Oct 5, 2018

max-wittig commented Oct 5, 2018

mythmon commented Oct 5, 2018

max-wittig commented Oct 5, 2018

peterbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbe left a comment

Choose a reason for hiding this comment

mythmon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbe commented Oct 8, 2018

max-wittig commented Oct 4, 2018 •

edited

Loading