Repository: peterbe/hashin

Issue:
Make it atomic
At the moment if you run `hashin` with more than one package, it deals with it one package at a time. See https://github.com/peterbe/hashin/blob/ccb661bd6548db78a0520b051f24a1dbba502065/hashin.py#L126-L127

Basically, it means, that it's the same thing doing:
```bash
$ hashin foo bar
```
as doing 
```bash
$ hashin foo
$ hashin bar
```

It's also clear when using the `--dry-run` option. You get an output like this:
```

--- Old
+++ New
@@ -1,9 +1,9 @@
 requests[security]==2.20.0 \
     --hash=sha256:99dcfdaaeb17caf6e526f32b6a7b780461512ab3f1d992187801694cba42770c \
     --hash=sha256:a84b8c9ab6239b578f22d1c21d51b696dcfe004032bb80ea832398d6909d7279
-kinto-attachment==5.0.0 \
-    --hash=sha256:4444e3c225d30cecdd5abd59afc9cc3b2412045b8b515a73e5b81a89638f3243 \
-    --hash=sha256:c54fd4d8a40498218c7b78f61fc5ce3eedead00a441af58fc12a12644e64410c
+kinto-attachment==6.0.0 \
+    --hash=sha256:12ce5c2ef718e7e31cef2e2a3bde771d9216f2cb014efba963e69cb709bcbbd1 \
+    --hash=sha256:fd54e979d3747be638f59de44a7f6523bed56d81961a438462b1346f49be5fe4
 boto3==1.9.16 \
     --hash=sha256:7d2ae0a759ede09474387dae246d8fe9c1f07e98fa3366ebffd0536571558261 \
     --hash=sha256:abc08c826c0628cb22fee2a2a5a21bad6a7f261042c652e4f994f376333cf52c

--- Old
+++ New
@@ -4,9 +4,9 @@
 kinto-attachment==5.0.0 \
     --hash=sha256:4444e3c225d30cecdd5abd59afc9cc3b2412045b8b515a73e5b81a89638f3243 \
     --hash=sha256:c54fd4d8a40498218c7b78f61fc5ce3eedead00a441af58fc12a12644e64410c
-boto3==1.9.16 \
-    --hash=sha256:7d2ae0a759ede09474387dae246d8fe9c1f07e98fa3366ebffd0536571558261 \
-    --hash=sha256:abc08c826c0628cb22fee2a2a5a21bad6a7f261042c652e4f994f376333cf52c
+boto3==1.9.31 \
+    --hash=sha256:03bdbd5bfc8c9f603877edf208a947b9e9d3d29268e3c3c46c3a8ee3d64ccda8 \
+    --hash=sha256:bc7b8e65b9612ee7ff03f71e765d0f380c8db096d898213bb4ead5b389c31cde
 botocore==1.12.31 \
     --hash=sha256:409edb0651313a04e93f2bfff2663ef84f742034f2099478d07b0c508b921c02 \
     --hash=sha256:dea459bfa0418689f2dfd8241739e7bb071d1cc944ffcc9cce0c19ecaf3612f3

```

That's when you do `hashin kinto-attachment boto3`. What you'd expect is one diff for ALL changes to be. 

Another benefit with being atomic is that the requirements file is only written to 1 single time. For example this:

```bash
$ touch requirements.txt
$ hashin requests neverheardofever
Traceback (most recent call last):
...
urllib.error.HTTPError: HTTP Error 404: Not Found
$ echo $?
1
$ cat /tmp/r.txt
requests==2.20.0 \
    --hash=sha256:99dcfdaaeb17caf6e526f32b6a7b780461512ab3f1d992187801694cba42770c \
    --hash=sha256:a84b8c9ab6239b578f22d1c21d51b696dcfe004032bb80ea832398d6909d7279
```

That's not nice. You get an error but actually did change the state of things. That's potentially very confusing. 

Discussion:

Patch:
diff --git a/hashin.py b/hashin.py
index 98d5c44..3ed9677 100755
--- a/hashin.py
+++ b/hashin.py
@@ -152,13 +152,12 @@ def run(specs, requirements_file, *args, **kwargs):
     if isinstance(specs, str):
         specs = [specs]
 
-    for spec in specs:
-        run_single_package(spec, requirements_file, *args, **kwargs)
+    run_packages(specs, requirements_file, *args, **kwargs)
     return 0
 
 
-def run_single_package(
-    spec,
+def run_packages(
+    specs,
     file,
     algorithm,
     python_versions=None,
@@ -166,46 +165,50 @@ def run_single_package(
     include_prereleases=False,
     dry_run=False,
 ):
-    restriction = None
-    if ";" in spec:
-        spec, restriction = [x.strip() for x in spec.split(";", 1)]
-    if "==" in spec:
-        package, version = spec.split("==")
-    else:
-        assert ">" not in spec and "<" not in spec
-        package, version = spec, None
-        # There are other ways to what the latest version is.
-
-    req = Requirement(package)
-
-    data = get_package_hashes(
-        package=req.name,
-        version=version,
-        verbose=verbose,
-        python_versions=python_versions,
-        algorithm=algorithm,
-        include_prereleases=include_prereleases,
-    )
-    package = data["package"]
-    # We need to keep this `req` instance for the sake of turning it into a string
-    # the correct way. But, the name might actually be wrong. Suppose the user
-    # asked for "Django" but on PyPI it's actually called "django", then we want
-    # correct that.
-    # We do that by modifying only the `name` part of the `Requirement` instance.
-    req.name = package
-
-    maybe_restriction = "" if not restriction else "; {0}".format(restriction)
-    new_lines = "{0}=={1}{2} \\\n".format(req, data["version"], maybe_restriction)
-    padding = " " * 4
-    for i, release in enumerate(data["hashes"]):
-        new_lines += "{0}--hash={1}:{2}".format(padding, algorithm, release["hash"])
-        if i != len(data["hashes"]) - 1:
-            new_lines += " \\"
-        new_lines += "\n"
+    assert isinstance(specs, list), type(specs)
+    all_new_lines = []
+    for spec in specs:
+        restriction = None
+        if ";" in spec:
+            spec, restriction = [x.strip() for x in spec.split(";", 1)]
+        if "==" in spec:
+            package, version = spec.split("==")
+        else:
+            assert ">" not in spec and "<" not in spec
+            package, version = spec, None
+            # There are other ways to what the latest version is.
+
+        req = Requirement(package)
+
+        data = get_package_hashes(
+            package=req.name,
+            version=version,
+            verbose=verbose,
+            python_versions=python_versions,
+            algorithm=algorithm,
+            include_prereleases=include_prereleases,
+        )
+        package = data["package"]
+        # We need to keep this `req` instance for the sake of turning it into a string
+        # the correct way. But, the name might actually be wrong. Suppose the user
+        # asked for "Django" but on PyPI it's actually called "django", then we want
+        # correct that.
+        # We do that by modifying only the `name` part of the `Requirement` instance.
+        req.name = package
+
+        maybe_restriction = "" if not restriction else "; {0}".format(restriction)
+        new_lines = "{0}=={1}{2} \\\n".format(req, data["version"], maybe_restriction)
+        padding = " " * 4
+        for i, release in enumerate(data["hashes"]):
+            new_lines += "{0}--hash={1}:{2}".format(padding, algorithm, release["hash"])
+            if i != len(data["hashes"]) - 1:
+                new_lines += " \\"
+            new_lines += "\n"
+        all_new_lines.append((package, new_lines))
 
     with open(file) as f:
         old_requirements = f.read()
-    requirements = amend_requirements_content(old_requirements, package, new_lines)
+    requirements = amend_requirements_content(old_requirements, all_new_lines)
     if dry_run:
         if verbose:
             _verbose("Dry run, not editing ", file)
@@ -226,30 +229,35 @@ def run_single_package(
             _verbose("Editing", file)
 
 
-def amend_requirements_content(requirements, package, new_lines):
-    regex = re.compile(
-        r"(^|\n|\n\r){0}==|(^|\n|\n\r){0}\[.*\]==".format(re.escape(package)),
-        re.IGNORECASE,
-    )
-    # if the package wasn't already there, add it to the bottom
-    if not regex.search(requirements):
-        # easy peasy
-        if requirements:
-            requirements = requirements.strip() + "\n"
-        requirements += new_lines.strip() + "\n"
-    else:
-        # need to replace the existing
-        lines = []
-        padding = " " * 4
-        for line in requirements.splitlines():
-            if regex.search(line):
-                lines.append(line)
-            elif lines and line.startswith(padding):
-                lines.append(line)
-            elif lines:
-                break
-        combined = "\n".join(lines + [""])
-        requirements = requirements.replace(combined, new_lines)
+def amend_requirements_content(requirements, all_new_lines):
+
+    # I wish we had types!
+    assert isinstance(all_new_lines, list), type(all_new_lines)
+
+    for package, new_lines in all_new_lines:
+        regex = re.compile(
+            r"(^|\n|\n\r){0}==|(^|\n|\n\r){0}\[.*\]==".format(re.escape(package)),
+            re.IGNORECASE,
+        )
+        # if the package wasn't already there, add it to the bottom
+        if not regex.search(requirements):
+            # easy peasy
+            if requirements:
+                requirements = requirements.strip() + "\n"
+            requirements += new_lines.strip() + "\n"
+        else:
+            # need to replace the existing
+            lines = []
+            padding = " " * 4
+            for line in requirements.splitlines():
+                if regex.search(line):
+                    lines.append(line)
+                elif lines and line.startswith(padding):
+                    lines.append(line)
+                elif lines:
+                    break
+            combined = "\n".join(lines + [""])
+            requirements = requirements.replace(combined, new_lines)
 
     return requirements
 


Test patch:
diff --git a/tests/test_cli.py b/tests/test_cli.py
index 4b7116c..f48b3de 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -217,14 +217,69 @@ def test_amend_requirements_content_new():
         + "\n"
     )
     new_lines = (
+        "autocompeter",
         """
 autocompeter==1.2.3 \\
     --hash=sha256:4d64ed1b9e0e73095f5cfa87f0e97ddb4c840049e8efeb7e63b46118ba1d623a
+    """.strip()
+        + "\n",
+    )
+    result = hashin.amend_requirements_content(requirements, [new_lines])
+    assert result == requirements + new_lines[1]
+
+
+def test_amend_requirements_content_multiple_merge():
+    requirements = (
+        """
+autocompeter==1.2.3 \\
+    --hash=sha256:4d64ed1b9e0e73095f5cfa87f0e97ddb4c840049e8efeb7e63b46118ba1d623a
+otherpackage==1.0.0 \\
+    --hash=sha256:cHay6ATFKumO3svU3B-8qBMYb-f1_dYlR4OgClWntEI
+# Comment here
+examplepackage==9.8.6 \\
+    --hash=sha256:33a5d0145e82326e781ddee1ad375f92cb84f8cfafea56e9504682adff64a5ee
+    """.strip()
+        + "\n"
+    )
+
+    all_new_lines = []
+    all_new_lines.append(
+        (
+            "autocompeter",
+            """
+autocompeter==1.3.0 \\
+    --hash=sha256:53929418a41295b526fbb68e43bc32fe93c3ef99c030b9e705caf1de486440de
+    """.strip()
+            + "\n",
+        )
+    )
+    all_new_lines.append(
+        (
+            "examplepackage",
+            """
+examplepackage==10.0.0 \\
+    --hash=sha256:fd54e979d3747be638f59de44a7f6523bed56d81961a438462b1346f49be5fe4
+    --hash=sha256:12ce5c2ef718e7e31cef2e2a3bde771d9216f2cb014efba963e69cb709bcbbd1
+    """.strip()
+            + "\n",
+        )
+    )
+
+    result = hashin.amend_requirements_content(requirements, all_new_lines)
+    expect = (
+        """
+autocompeter==1.3.0 \\
+    --hash=sha256:53929418a41295b526fbb68e43bc32fe93c3ef99c030b9e705caf1de486440de
+otherpackage==1.0.0 \\
+    --hash=sha256:cHay6ATFKumO3svU3B-8qBMYb-f1_dYlR4OgClWntEI
+# Comment here
+examplepackage==10.0.0 \\
+    --hash=sha256:fd54e979d3747be638f59de44a7f6523bed56d81961a438462b1346f49be5fe4
+    --hash=sha256:12ce5c2ef718e7e31cef2e2a3bde771d9216f2cb014efba963e69cb709bcbbd1
     """.strip()
         + "\n"
     )
-    result = hashin.amend_requirements_content(requirements, "autocompeter", new_lines)
-    assert result == requirements + new_lines
+    assert result == expect
 
 
 def test_amend_requirements_content_replacement():
@@ -235,15 +290,18 @@ autocompeter==1.2.2
     """.strip()
         + "\n"
     )
+
     new_lines = (
+        "autocompeter",
         """
 autocompeter==1.2.3
     --hash=sha256:4d64ed1b9e0e73095f5cfa87f0e97ddb4c840049e8efeb7e63b46118ba1d623a
     """.strip()
-        + "\n"
+        + "\n",
     )
-    result = hashin.amend_requirements_content(requirements, "autocompeter", new_lines)
-    assert result == new_lines
+
+    result = hashin.amend_requirements_content(requirements, [new_lines])
+    assert result == new_lines[1]
 
 
 def test_amend_requirements_content_replacement_single_to_multi():
@@ -257,14 +315,15 @@ autocompeter==1.2.2 --hash=sha256:33a5d0145e82326e781ddee1ad375f92cb84f8cfafea56
         + "\n"
     )
     new_lines = (
+        "autocompeter",
         """
 autocompeter==1.2.3
     --hash=sha256:4d64ed1b9e0e73095f5cfa87f0e97ddb4c840049e8efeb7e63b46118ba1d623a
     """.strip()
-        + "\n"
+        + "\n",
     )
-    result = hashin.amend_requirements_content(requirements, "autocompeter", new_lines)
-    assert result == new_lines
+    result = hashin.amend_requirements_content(requirements, [new_lines])
+    assert result == new_lines[1]
 
 
 def test_amend_requirements_content_replacement_2():
@@ -277,14 +336,15 @@ autocompeter==1.2.2 \\
         + "\n"
     )
     new_lines = (
+        "autocompeter",
         """
 autocompeter==1.2.3 \\
     --hash=sha256:4d64ed1b9e0e73095f5cfa87f0e97ddb4c840049e8efeb7e63b46118ba1d623a
     """.strip()
-        + "\n"
+        + "\n",
     )
-    result = hashin.amend_requirements_content(requirements, "autocompeter", new_lines)
-    assert result == new_lines
+    result = hashin.amend_requirements_content(requirements, [new_lines])
+    assert result == new_lines[1]
 
 
 def test_amend_requirements_content_replacement_amonst_others():
@@ -303,12 +363,15 @@ autocompeter==1.2.2 \\
     """.strip()
         + "\n"
     )
-    new_lines = """
+    new_lines = (
+        "autocompeter",
+        """
 autocompeter==1.2.3 \\
     --hash=sha256:4d64ed1b9e0e73095f5cfa87f0e97ddb4c840049e8efeb7e63b46118ba1d623a
-    """.strip()
-    result = hashin.amend_requirements_content(requirements, "autocompeter", new_lines)
-    assert result == previous + new_lines
+    """.strip(),
+    )
+    result = hashin.amend_requirements_content(requirements, [new_lines])
+    assert result == previous + new_lines[1]
 
 
 def test_amend_requirements_content_replacement_amonst_others_2():
@@ -327,12 +390,15 @@ autocompeter==1.2.2
     """.strip()
         + "\n"
     )
-    new_lines = """
+    new_lines = (
+        "autocompeter",
+        """
 autocompeter==1.2.3  \\
     --hash=256:4d64ed1b9e0e73095f5cfa87f0e97ddb4c840049e8efeb7e63b46118ba1d623a
-    """.strip()
-    result = hashin.amend_requirements_content(requirements, "autocompeter", new_lines)
-    assert result == previous + new_lines
+        """.strip(),
+    )
+    result = hashin.amend_requirements_content(requirements, [new_lines])
+    assert result == previous + new_lines[1]
 
 
 def test_amend_requirements_content_new_similar_name():
@@ -352,17 +418,18 @@ selenium==2.52.0 \
     """.strip()
         + "\n"
     )
-    new_lines = """
+    new_lines = (
+        "selenium",
+        """
 selenium==2.53.1 \
     --hash=sha256:b1af142650ed7025f906349ae0d7ed1f1a1e635e6ce7ac67e2b2f854f9f8fdc1 \
     --hash=sha256:53929418a41295b526fbb68e43bc32fe93c3ef99c030b9e705caf1de486440de
-    """.strip()
-    result = hashin.amend_requirements_content(
-        previous_1 + previous_2, "selenium", new_lines
+        """.strip(),
     )
+    result = hashin.amend_requirements_content(previous_1 + previous_2, [new_lines])
     assert previous_1 in result
     assert previous_2 not in result
-    assert new_lines in result
+    assert new_lines[1] in result
 
 
 def test_run(murlopen, tmpfile, capsys):
@@ -462,6 +529,57 @@ def test_run(murlopen, tmpfile, capsys):
         )
 
 
+def test_run_atomic_not_write_with_error_on_last_package(murlopen, tmpfile):
+    def mocked_get(url, **options):
+
+        if url == "https://pypi.org/pypi/hashin/json":
+            return _Response(
+                {
+                    "info": {"version": "0.10", "name": "hashin"},
+                    "releases": {
+                        "0.10": [
+                            {
+                                "url": "https://pypi.org/packages/2.7/p/hashin/hashin-0.10-py2-none-any.whl",
+                                "digests": {"sha256": "aaaaa"},
+                            },
+                            {
+                                "url": "https://pypi.org/packages/3.3/p/hashin/hashin-0.10-py3-none-any.whl",
+                                "digests": {"sha256": "bbbbb"},
+                            },
+                            {
+                                "url": "https://pypi.org/packages/source/p/hashin/hashin-0.10.tar.gz",
+                                "digests": {"sha256": "ccccc"},
+                            },
+                        ]
+                    },
+                }
+            )
+
+        if url == "https://pypi.org/pypi/gobblygook/json":
+            if HTTPError:
+                raise HTTPError(url, 404, "Page not found", {}, None)
+            else:
+                return _Response({}, status_code=404)
+
+        raise NotImplementedError(url)
+
+    murlopen.side_effect = mocked_get
+
+    with tmpfile() as filename:
+        with open(filename, "w") as f:
+            f.write("")
+
+        with pytest.raises(hashin.PackageNotFoundError):
+            hashin.run(["hashin", "gobblygook"], filename, "sha256", verbose=True)
+
+        with open(filename) as f:
+            output = f.read()
+            # Crucial that nothing was written to the file.
+            # The first package would find some new requirements but the second
+            # package should cancel the write.
+            assert output == ""
+
+
 def test_run_without_specific_version(murlopen, tmpfile):
     def mocked_get(url, **options):
         if url == "https://pypi.org/pypi/hashin/json":
@@ -865,6 +983,77 @@ def test_run_dry(murlopen, tmpfile, capsys):
     assert "+--hash=sha256:aaaaa" in out_lines[4].replace(" ", "")
 
 
+def test_run_dry_multiple_packages(murlopen, tmpfile, capsys):
+    """dry run should edit the requirements.txt file and print
+    hashes and package name in the console
+    """
+
+    def mocked_get(url, **options):
+        if url == "https://pypi.org/pypi/hashin/json":
+            return _Response(
+                {
+                    "info": {"version": "0.11", "name": "hashin"},
+                    "releases": {
+                        "0.11": [
+                            {
+                                "url": "https://pypi.org/packages/source/p/hashin/hashin-0.11.tar.gz",
+                                "digests": {"sha256": "bbbbb"},
+                            }
+                        ],
+                        "0.10": [
+                            {
+                                "url": "https://pypi.org/packages/source/p/hashin/hashin-0.10.tar.gz",
+                                "digests": {"sha256": "aaaaa"},
+                            }
+                        ],
+                    },
+                }
+            )
+        if url == "https://pypi.org/pypi/requests/json":
+            return _Response(
+                {
+                    "info": {"version": "1.2.4", "name": "requests"},
+                    "releases": {
+                        "1.2.4": [
+                            {
+                                "url": "https://pypi.org/packages/source/p/requests/requests-1.2.4.tar.gz",
+                                "digests": {"sha256": "dededede"},
+                            }
+                        ]
+                    },
+                }
+            )
+
+        raise NotImplementedError(url)
+
+    murlopen.side_effect = mocked_get
+
+    with tmpfile() as filename:
+        with open(filename, "w") as f:
+            f.write("")
+
+        retcode = hashin.run(
+            ["hashin", "requests"], filename, "sha256", verbose=False, dry_run=True
+        )
+        assert retcode == 0
+
+        # verify that nothing has been written to file
+        with open(filename) as f:
+            output = f.read()
+        assert not output
+
+    # Check dry run output
+    captured = capsys.readouterr()
+    output = captured.out
+    out_lines = output.splitlines()
+    assert output.count("Old") == 1
+    assert output.count("New") == 1
+    assert "+hashin==0.11" in out_lines[3]
+    assert "+--hash=sha256:bbbbb" in out_lines[4].replace(" ", "")
+    assert "+requests==1.2.4" in out_lines[5]
+    assert "+--hash=sha256:dededede" in out_lines[6].replace(" ", "")
+
+
 def test_run_pep_0496(murlopen, tmpfile):
     """
     Properly pass through specifiers which look like: