Skip to content

Fix requirement produced for git URL in extra #358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jaraco merged 4 commits into from
Dec 16, 2021
Merged

Fix requirement produced for git URL in extra #358

jaraco merged 4 commits into from
Dec 16, 2021

Conversation

@hauntsaninja
Copy link
Contributor

@hauntsaninja hauntsaninja commented Dec 15, 2021 •
edited
Loading

Fixes #357 , as suggested by @uranusjr

@jaraco
Copy link
Member

jaraco commented Dec 16, 2021

I cherry-picked the tests onto main and verified that they do indeed fail without the patch:

_______________________________________________ APITests.test_more_complex_deps_requires_text _______________________________________________

self = <tests.test_api.APITests testMethod=test_more_complex_deps_requires_text>

    def test_more_complex_deps_requires_text(self):
        requires = textwrap.dedent(
            """
            dep1
            dep2
    
            [:python_version < "3"]
            dep3
    
            [extra1]
            dep4
            dep5@ git+https://example.com/python/dep.git@v1.0.0
    
            [extra2:python_version < "3"]
            dep6
            """
        )
        deps = sorted(Distribution._deps_from_requires_text(requires))
        expected = [
            'dep1',
            'dep2',
            'dep3 ; python_version < "3"',
            'dep4 ; extra == "extra1"',
            'dep5@ git+https://example.com/python/dep.git@v1.0.0 ; extra == "extra1"',
            'dep6 ; (python_version < "3") and extra == "extra2"',
        ]
        # It's important that the environment marker expression be
        # wrapped in parentheses to avoid the following 'and' binding more
        # tightly than some other part of the environment expression.
    
>       assert deps == expected
E       AssertionError: assert ['dep1', 'dep... == "extra2"'] == ['dep1', 'dep... == "extra2"']
E         At index 2 diff: 'dep3; python_version < "3"' != 'dep3 ; python_version < "3"'
E         Use -v to get the full diff

tests/test_api.py:267: AssertionError

I do think since the failure doesn't specifically call out why it failed, there should be a comment similar to the "It's important..." comment that's already there, explaining what we learned in #357.

@jaraco
Copy link
Member

jaraco commented Dec 16, 2021

The tests are failing in Python 3.6 due to mypy complaining about no importlib.resources module. That test wasn't failing before, so I suspect something has changed in mypy.

@jaraco jaraco mentioned this pull request Dec 16, 2021
Closed
jaraco
jaraco reviewed Dec 16, 2021
View reviewed changes
@jaraco
Copy link
Member

jaraco commented Dec 16, 2021

I do think since the failure doesn't specifically call out why it failed, there should be a comment similar to the "It's important..." comment that's already there, explaining what we learned in #357.

I no longer thing it's important to call out the why in the tests because the implementation calls out the rationale.

@jaraco jaraco merged commit 134ed7f into python:main Dec 16, 2021
@hauntsaninja hauntsaninja deleted the fixurl branch December 16, 2021 19:03
uranusjr
uranusjr reviewed Dec 17, 2021
View reviewed changes
importlib_metadata/__init__.py
Ref python/importlib_metadata#357.
"""
# '@' is uniquely indicative of a url_req.
return ' ' * ('@' in req)
Copy link
Contributor

@uranusjr uranusjr Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this is not correct because @ can be found in a version here; the version format in an egg-info is not PEP 508 compliant (it predates the standard). But adding a space is always fine, so no bugs will come out of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaraco jaraco jaraco left review comments

+1 more reviewer

@uranusjr uranusjr uranusjr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

InvalidRequirement produced for git URL in extra

3 participants

@hauntsaninja @jaraco @uranusjr