-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
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 "" |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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..
There was a problem hiding this 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.
76605d8
to
9b10b93
Compare
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.