Repository: peterbe/hashin

Issue:
Hashin support for not distinguishing between underscored/hyphenated
pypi appears to follow guidelines outlined in PEP8 around package names preferring to hyphenate them as opposed to underscore them

https://www.python.org/dev/peps/pep-0008/#package-and-module-names

"""
Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability. Python packages should also have short, all-lowercase names, although the use of underscores is discouraged.
"""

Note there is a distinction between a module and a package. `hashin` is operating on packages. 

Regardless of the naming choices, pypi appears to support both underscores and hyphens in packages. This is likely being done so-as to prevent major breaking changes across existing software.

Consider, for example, if pypi drew a hard line on this, switched all package names to hyphens, and disallowed underscores; a tidal wave of broken software would exist based on this decision.

So it looks like pypi supports both and (may?) support both forever. However, when you use hashin to get the package of choice, you're always returned the "correct" one; ie, the hyphenated version.

There exists a case where a user may have an existing (underscored) package definition, and hashin should behave to correct this to use the hyphenated version.

For instance, if the user's requirements file has the following (truncated sha's)

```
python_memcached==1.59 \
    --hash=sha256:4dac64916871bd35502 \
    --hash=sha256:a2e28637be13ee0bf1a8
```

Then upon running either of the following

* `hashin python_memcached==1.6.0 -r requirements.txt`
* `hashin python-memcached==1.6.0 -r requirements.txt`

The expected change to the requirements file would be

```
python-memcached==1.60 \
    --hash=sha256:x00c64916871bd35502 \
    --hash=sha256:x02e28637be13ee0bf1a
```

Today, however, the behavior is that `hashin` will do the following.

* Update, in place, if python_memcached is hashed
* Append if python-memcached is hashed

This issue proposes that hashin normalize this behavior to prefer (in all cases) the hyphenated name. Therefore,

* If python_memcached is hashed, replace python_memcached with python-memcached, and update hashes in place for python-memcached
* If python-memcached is hashed, update hashes in place for python-memcached

Discussion:
caphrim007: @peterbe ready for review
caphrim007: Fixes #116
caphrim007: @peterbe I will send updates shortly. thanks for reviewing!
caphrim007: changes made. Ready for review
peterbe: Travis isn't reporting checks here on the PR, but: https://travis-ci.org/github/peterbe/hashin/builds/698623654 is all green!

Patch:
diff --git a/hashin.py b/hashin.py
index 515e037..df1a229 100755
--- a/hashin.py
+++ b/hashin.py
@@ -388,17 +388,39 @@ def amend_requirements_content(requirements, all_new_lines):
     padding = " " * 4
 
     def is_different_lines(old_lines, new_lines, indent):
+        # This regex is used to only temporarily normalize the names of packages
+        # in the lines being compared. This results in "old" names matching
+        # "new" names so that hashin correctly replaces them when it looks for
+        # them.
+        match_delims = re.compile(r"[-_]")
+
         # This assumes that the package is already mentioned in the old
         # requirements. Now we just need to double-check that its lines are
         # different.
         # The 'new_lines` is what we might intend to replace it with.
-        old = set([l.strip(" \\") for l in old_lines])
+        old = set([match_delims.sub("-", line.strip(" \\")) for line in old_lines])
         new = set([indent + x.strip(" \\") for x in new_lines])
         return old != new
 
     for package, old_name, new_text in all_new_lines:
+        # The call to `escape` will turn hyphens into escaped hyphens
+        #
+        # ex.
+        #   -       becomes     \\-
+        #
+        escaped = re.escape(old_name)
+
+        # This changes those escaped hypens into a pattern to match
+        #
+        # ex.
+        #   \\-     becomes     [-_]
+        #
+        # This is necessary so that hashin will correctly find underscored (old)
+        # and hyphenated (new) package names so that it will correctly replace an
+        # old name with the new name when there is a version update.
+        escape_replaced = escaped.replace("\\-", "[-_]")
         regex = re.compile(
-            r"^(?P<indent>[ \t]*){0}(\[.*\])?==".format(re.escape(old_name)),
+            r"^(?P<indent>[ \t]*){0}(\[.*\])?==".format(escape_replaced),
             re.IGNORECASE | re.MULTILINE,
         )
         # if the package wasn't already there, add it to the bottom
diff --git a/tox.ini b/tox.ini
index 0d54cd5..5c36c84 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,5 +1,5 @@
 [tox]
-envlist = py{27,34,35,36}, lint, restlint
+envlist = py{27,34,35,36,37}, lint, restlint
 
 [travis]
 python =
@@ -7,6 +7,7 @@ python =
   3.4: py34
   3.5: py35
   3.6: py36, lint, restlint
+  3.7: py37, lint, restlint
 
 [testenv]
 usedevelop = True


Test patch:
diff --git a/tests/test_cli.py b/tests/test_cli.py
index 6bf9cc6..7f5d119 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -294,6 +294,26 @@ autocompeter==1.2.3 \\
     assert result == requirements + new_lines[2]
 
 
+def test_amend_requirements_content_new_2():
+    requirements = (
+        """
+# empty so far
+    """.strip()
+        + "\n"
+    )
+    new_lines = (
+        "discogs-client",
+        "Discogs_client",
+        """
+discogs-client==1.1 \\
+    --hash=sha256:4d64ed1b9e0e73095f5cfa87f0e97ddb4c840049e8efeb7e63b46118ba1d623a
+    """.strip()
+        + "\n",
+    )
+    result = hashin.amend_requirements_content(requirements, [new_lines])
+    assert result == requirements + new_lines[2]
+
+
 def test_amend_requirements_different_old_name():
     requirements = (
         """
@@ -2665,3 +2685,222 @@ def test_interactive_upgrade_request_force_yes(capsys):
             "hashin", old_version, new_version, force_yes=True
         )
         assert result == "YES"
+
+
+class TestIssue116:
+    """Tests for validating fix related to GH issue #116
+
+    https://github.com/peterbe/hashin/issues/116
+    """
+
+    def test_amend_requirements_content_new_without_env_markers(self):
+        requirements = (
+            """
+# empty so far
+        """.strip()
+            + "\n"
+        )
+        new_lines = (
+            "discogs-client",
+            "discogs-client",
+            """
+discogs-client==1.1 \\
+    --hash=sha256:4d64ed1b9e0e73095f5cfa87f0e97ddb4c840049e8efeb7e63b46118ba1d623a
+        """.strip()
+            + "\n",
+        )
+        result = hashin.amend_requirements_content(requirements, [new_lines])
+        assert result == requirements + new_lines[2]
+
+    def test_amend_requirements_content_new_with_env_markers(self):
+        requirements = (
+            """
+# empty so far
+        """.strip()
+            + "\n"
+        )
+        new_lines = (
+            "discogs-client; python_version <= '3.4'",
+            "discogs-client; python_version <= '3.4'",
+            """
+discogs-client==1.1; python_version <= '3.4' \\
+    --hash=sha256:4d64ed1b9e0e73095f5cfa87f0e97ddb4c840049e8efeb7e63b46118ba1d623a
+        """.strip()
+            + "\n",
+        )
+        result = hashin.amend_requirements_content(requirements, [new_lines])
+        assert result == requirements + new_lines[2]
+
+    def test_amend_requirements_different_old_name_without_env_markers(self):
+        requirements = (
+            """
+readme_renderer==25.0 \\
+    --hash=sha256:1b6d8dd1673a0b293766b4106af766b6eff3654605f9c4f239e65de6076bc222 \\
+    --hash=sha256:e67d64242f0174a63c3b727801a2fff4c1f38ebe5d71d95ff7ece081945a6cd4
+
+        """.strip()
+            + "\n"
+        )
+
+        new_lines = (
+            "readme-renderer",
+            "readme-renderer",
+            """
+readme-renderer==26.0 \\
+    --hash=sha256:cbe9db71defedd2428a1589cdc545f9bd98e59297449f69d721ef8f1cfced68d \\
+    --hash=sha256:cc4957a803106e820d05d14f71033092537a22daa4f406dfbdd61177e0936376
+
+        """.strip()
+            + "\n",
+        )
+        result = hashin.amend_requirements_content(requirements, [new_lines])
+        assert result == new_lines[2]
+
+    def test_amend_requirements_different_old_name_with_env_markers(self):
+        requirements = (
+            """
+readme_renderer==25.0; python_version <= "3.4" \\
+    --hash=sha256:1b6d8dd1673a0b293766b4106af766b6eff3654605f9c4f239e65de6076bc222 \\
+    --hash=sha256:e67d64242f0174a63c3b727801a2fff4c1f38ebe5d71d95ff7ece081945a6cd4
+
+        """.strip()
+            + "\n"
+        )
+
+        new_lines = (
+            "readme-renderer",
+            "readme-renderer",
+            """
+readme-renderer==26.0; python_version <= "3.4" \\
+    --hash=sha256:cbe9db71defedd2428a1589cdc545f9bd98e59297449f69d721ef8f1cfced68d \\
+    --hash=sha256:cc4957a803106e820d05d14f71033092537a22daa4f406dfbdd61177e0936376
+
+        """.strip()
+            + "\n",
+        )
+        result = hashin.amend_requirements_content(requirements, [new_lines])
+        assert result == new_lines[2]
+
+    def test_amend_requirements_without_env_markers_same_version(self):
+        requirements = (
+            """
+readme_renderer==25.0 \\
+    --hash=sha256:1b6d8dd1673a0b293766b4106af766b6eff3654605f9c4f239e65de6076bc222 \\
+    --hash=sha256:e67d64242f0174a63c3b727801a2fff4c1f38ebe5d71d95ff7ece081945a6cd4
+
+        """.strip()
+            + "\n"
+        )
+
+        new_lines = (
+            "readme-renderer",
+            "readme-renderer",
+            """
+readme_renderer==25.0 \\
+    --hash=sha256:1b6d8dd1673a0b293766b4106af766b6eff3654605f9c4f239e65de6076bc222 \\
+    --hash=sha256:e67d64242f0174a63c3b727801a2fff4c1f38ebe5d71d95ff7ece081945a6cd4
+
+        """.strip()
+            + "\n",
+        )
+        result = hashin.amend_requirements_content(requirements, [new_lines])
+        assert result == requirements
+
+    def test_amend_requirements_with_env_markers_same_version(self):
+        requirements = (
+            """
+readme_renderer==25.0; python_version <= "3.4" \\
+    --hash=sha256:1b6d8dd1673a0b293766b4106af766b6eff3654605f9c4f239e65de6076bc222 \\
+    --hash=sha256:e67d64242f0174a63c3b727801a2fff4c1f38ebe5d71d95ff7ece081945a6cd4
+
+        """.strip()
+            + "\n"
+        )
+
+        new_lines = (
+            "readme-renderer",
+            "readme-renderer",
+            """
+readme_renderer==25.0; python_version <= "3.4" \\
+    --hash=sha256:1b6d8dd1673a0b293766b4106af766b6eff3654605f9c4f239e65de6076bc222 \\
+    --hash=sha256:e67d64242f0174a63c3b727801a2fff4c1f38ebe5d71d95ff7ece081945a6cd4
+
+        """.strip()
+            + "\n",
+        )
+        result = hashin.amend_requirements_content(requirements, [new_lines])
+        assert result == requirements
+
+    def test_run_old_name_no_version_change(self, murlopen, tmpfile, capsys):
+        def mocked_get(url, **options):
+            if url == "https://pypi.org/pypi/readme_renderer/json":
+                return _Response(
+                    {
+                        "info": {"version": "0.25", "name": "readme-renderer"},
+                        "releases": {
+                            "25.0": [
+                                {
+                                    "url": "https://pypi.org/packages/source/p/readme-renderer/readme_renderer-25.0.tar.gz",
+                                    "digests": {"sha256": "bbbbb"},
+                                }
+                            ],
+                            "26.0": [
+                                {
+                                    "url": "https://pypi.org/packages/source/p/readme-renderer/readme_renderer-26.0.tar.gz",
+                                    "digests": {"sha256": "aaaaa"},
+                                }
+                            ],
+                        },
+                    }
+                )
+
+        murlopen.side_effect = mocked_get
+
+        with tmpfile() as filename:
+            with open(filename, "w") as f:
+                f.write("readme_renderer==25.0 \\\n")
+                f.write("    --hash=sha256:bbbbb\n")
+                f.write("\n")
+
+            retcode = hashin.run("readme_renderer==25.0", filename, "sha256")
+            assert retcode == 0
+            with open(filename) as f:
+                output = f.read()
+            assert "readme_renderer==25.0" in output
+
+    def test_run_old_name_new_version_change(self, murlopen, tmpfile, capsys):
+        def mocked_get(url, **options):
+            if url == "https://pypi.org/pypi/readme_renderer/json":
+                return _Response(
+                    {
+                        "info": {"version": "0.25", "name": "readme-renderer"},
+                        "releases": {
+                            "25.0": [
+                                {
+                                    "url": "https://pypi.org/packages/source/p/readme-renderer/readme_renderer-25.0.tar.gz",
+                                    "digests": {"sha256": "bbbbb"},
+                                }
+                            ],
+                            "26.0": [
+                                {
+                                    "url": "https://pypi.org/packages/source/p/readme-renderer/readme_renderer-26.0.tar.gz",
+                                    "digests": {"sha256": "aaaaa"},
+                                }
+                            ],
+                        },
+                    }
+                )
+
+        murlopen.side_effect = mocked_get
+
+        with tmpfile() as filename:
+            with open(filename, "w") as f:
+                f.write("readme_renderer==26.0 \\\n")
+                f.write("    --hash=sha256:bbbbb\n")
+                f.write("\n")
+
+            retcode = hashin.run("readme_renderer==26.0", filename, "sha256")
+            assert retcode == 0
+            with open(filename) as f:
+                output = f.read()
+            assert "readme-renderer==26.0" in output