Fix capitalization #452
Fix capitalization #452
Conversation
The code removed is within an if-statement that only returns True when no upgrades are selected. This means that upgrade_pgks_by_key is always empty here.
|
Any idea why the build is failing? I can't access the Travis log. |
|
The project has moved to the jazzband organization and it seems that past builds have not followed from nvie. Pushing another commit (or amending the last commit) should trigger a build in the new organization. |
|
If you change the url to jazzband from nvie, it should show the build. |
|
https://travis-ci.org/jazzband/pip-tools/builds/199895230 Note however that the "This branch has no conflicts with the base branch" message is flawed here. Rebasing on master should help. |
|
This looks like a good fix for the capitalization behavior. In the original issue (#431), I had raised some questions about some possible cache-related weirdness. Should I create another GitHub issue for that? Either way (new issue or no), I will need to go back to try and reproduce the issue. |
|
Yes I have read a lot of code, but I could not really find where the caching actually takes place. This seems to be part of another package. |
| @@ -43,7 +43,7 @@ def test_format_requirement_annotation_case_sensitive(from_line, writer): | |||
| assert (writer._format_requirement(ireq, | |||
| reverse_dependencies, | |||
| primary_packages=[]) == | |||
| 'Test==1.2 ' + comment('# via xyz')) | |||
There was a problem hiding this comment.
This test is made to explicitly validate that case is NOT changed. (test is named test_format_requirement_annotation_case_sensitive). Are we sure this is ok ?
There was a problem hiding this comment.
Case sensitivity is not the same as unchanged case. On the other hand, neither pip, nor pypi, nor pip-tools (as it uses pip itself) are case sensitive. Maybe it's better to either just get rid of this test, or rename it, or even write another one?
There was a problem hiding this comment.
Yes that makes sense.
Going forward, I think adding and maintaining tests is really important, particularly in this new "distributed" maintainership model (jazzband) that pip-tools is using. I know it is more work, but it is better to spend time locally than distribute the debugging effort on the internet :-).
Maybe this should be included in the 1.8.1 release I am preparing. A release candidate 1 is out already.
|
I do have a feeling we should document this case-insensitivity somewhere, although I have no idea where. |
|
Thanks @klaplong ! |
This fixes pypa#2113. This bug was introduced in <jazzband/pip-tools#452> as a band-aid fix to <jazzband/pip-tools#431>. Pipenv then copied that code in <pypa@2553ebc#diff-b56b95ccea8595a0f6f24ea753842976>, and inherited this latent bug. Maybe the right fix is for pypa/packaging to lowercase the name? There's a comment here <https://github.com/pypa/packaging/blob/16.8/packaging/requirements.py#L86> about normalizing the requirement's name, which might be what this is referring to.
This fixes pypa#2113. This bug was introduced in <jazzband/pip-tools#452> as a band-aid fix to <jazzband/pip-tools#431>. Pipenv then copied that code in <pypa@2553ebc#diff-b56b95ccea8595a0f6f24ea753842976>, and inherited this latent bug. Maybe the right fix is for pypa/packaging to lowercase the name? There's a comment here <https://github.com/pypa/packaging/blob/16.8/packaging/requirements.py#L86> about normalizing the requirement's name, which might be what this is referring to.
This fixes pypa#2113. This bug was introduced in <jazzband/pip-tools#452> as a band-aid fix to <jazzband/pip-tools#431>. Pipenv then copied that code in <pypa@2553ebc#diff-b56b95ccea8595a0f6f24ea753842976>, and inherited this latent bug. Maybe the right fix is for pypa/packaging to lowercase the name? There's a comment here <https://github.com/pypa/packaging/blob/16.8/packaging/requirements.py#L86> about normalizing the requirement's name, which might be what this is referring to.
This fixes pypa#2113. This bug was introduced in <jazzband/pip-tools#452> as a band-aid fix to <jazzband/pip-tools#431>. Pipenv then copied that code in <pypa@2553ebc#diff-b56b95ccea8595a0f6f24ea753842976>, and inherited this latent bug. Maybe the right fix is for pypa/packaging to lowercase the name? There's a comment here <https://github.com/pypa/packaging/blob/16.8/packaging/requirements.py#L86> about normalizing the requirement's name, which might be what this is referring to.
This fixes pypa#2113. This bug was introduced in <jazzband/pip-tools#452> as a band-aid fix to <jazzband/pip-tools#431>. Pipenv then copied that code in <pypa@2553ebc#diff-b56b95ccea8595a0f6f24ea753842976>, and inherited this latent bug. Maybe the right fix is for pypa/packaging to lowercase the name? There's a comment here <https://github.com/pypa/packaging/blob/16.8/packaging/requirements.py#L86> about normalizing the requirement's name, which might be what this is referring to. To test this, I invented a new, very simple python package called `depends-on-marked-package`. The setup.py for this package is just: ```python import setuptools setuptools.setup( name="depends-on-marked-package", version="0.0.1", packages=setuptools.find_packages(), install_requires=['pytz; platform_python_implementation=="CPython"'], ) ``` This is a simplified version of gevent's setup.py's install_requires upon greenlet.
Fixed #431