Skip to content
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.

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

Implement the new resolver's Requirement class #7843

Merged
merged 1 commit into from
Mar 11, 2020

Conversation

pfmoore
Copy link
Member

@pfmoore pfmoore commented Mar 10, 2020

This is the initial implementation of the new resolver's Requirement class. It isn't used anywhere in pip yet, further PRs will add the other resolver classes and link them together.

The tests here are very basic "does this thing work?" tests, more detailed functionality tests will be added when the full class structure is present.

@pfmoore
Copy link
Member Author

pfmoore commented Mar 10, 2020

Ping @pradyunsg @uranusjr. Getting this in here for review. I think it's good enough to go.

Candidate will be coming soon, but I want to do some tidying up and simplification before creating a PR.

assert self._ireq.req is None or self._ireq.name is None, \
"Unnamed requirement has a name"
# TODO: Get the candidate and use its name...
return ""
Copy link
Member

Choose a reason for hiding this comment

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

Are there cases where name wouldn’t be called? I can’t think of any. I am tempted to suggest always NamedRequirement instead, by getting the candidate very eagerly in make_requirement and assigning it to the InstallRequirement.

Copy link
Member

@uranusjr uranusjr Mar 11, 2020

Choose a reason for hiding this comment

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

I.e. something like

def make_requirement(install_req):
    # type: (InstallRequirement) -> Requirement
    if not install_req.link:
        return VersionedRequirement(install_req)
    if not install_req.req:
        install_req.req = _parse_link_into_req(install_req.link)  # TODO: Implement me.
    if install_req.req.name:
        return NamedRequirement(install_req, candidate=None)
    candidate = _make_candidate(install_req)  # TODO: Implement me.
    install_req.req.name = candidate.name
    return NamedRequirement(install_req, candidate)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, but without a "proper" candidate object, I can't do this without the tests failing (see the test that currently asserts that a sdist has a name of ""). So I plan on addressing this in one of the follow up PRs (either the one that implements candidates, or more likely a further PR that tidies up all the links).

In hindsight, I'm not convinced that splitting up the original change from #7799 was worthwhile, it's created a lot of busy-work, and I don't think the resulting PRs are that much more reviewable. But adding the tests was (IMO) a definite benefit, so I'm not going to worry too much..

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

On the ground that we need to get something in to get things started.

@pfmoore pfmoore force-pushed the resolvelib_requirement branch from 76605d8 to 9b10b93 Compare March 11, 2020 11:38
@pfmoore pfmoore merged commit 10f4157 into pypa:master Mar 11, 2020
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Apr 15, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2020
@pfmoore pfmoore deleted the resolvelib_requirement branch January 24, 2021 11:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants