8000 New Resolver: Make sure candidates are prepared after resolution by uranusjr · Pull Request #8000 · pypa/pip · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

New Resolver: Make sure candidates are prepared after resolution #8000

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
merged 2 commits into from
Apr 10, 2020
Merged

New Resolver: Make sure candidates are prepared after resolution #8000

merged 2 commits into from
Apr 10, 2020

Conversation

uranusjr
Copy link
Member
@uranusjr uranusjr commented Apr 8, 2020

This works because self.dist triggers a prepare method, but looks like a hack. Not sure how to do this best :(

@uranusjr uranusjr added the skip news Does not need a NEWS file entry (eg: trivial changes) label Apr 8, 2020
@pfmoore
Copy link
Member
pfmoore commented Apr 8, 2020

Refactor self.dist as

@property
def dist(self):
    if self._dist is None:
        self._dist = self.prepare() # or even have self.prepare() set self._dist directly.
    return self._dist

and use self.prepare() here?

@pradyunsg
Copy link
Member
pradyunsg commented Apr 8, 2020 •
edited
Loading

I think a good way to handle this would be along the lines of what Paul is suggesting, but including the conditional in the common-function's body as well.

     @property
     def dist(self):
         self._ensure_prepared()
         return self._dist

     def _ensure_prepared(self):
         if self._dist is not None:
             return
         ...

     def get_install_requirement(self):
         self._ensure_prepared()
         return self._ireq

A similar pattern is used in the Configuration class as well.

@uranusjr
Copy link
Member Author
uranusjr commented Apr 8, 2020

I opted to move the if check into the method (as @pradyunsg suggested), and also renamed get_abstract_distribution() to _prepare_abstract_distribution() to make it clearer this method is a part of the preparation.

pradyunsg
pradyunsg approved these changes Apr 9, 2020
View reviewed changes
Copy link
Member
@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

This LGTM but I do think we should be weary of spreading the use of the verb "prepare".

@uranusjr uranusjr changed the title Make sure candidates are prepared after resolution New Resolver: Make sure candidates are prepared after resolution Apr 9, 2020
@uranusjr uranusjr mentioned this pull request Apr 10, 2020
Merged
@pfmoore pfmoore merged commit 6c97645 into pypa:master Apr 10, 2020
@uranusjr uranusjr deleted the new-resolver-normal-test-progress branch April 10, 2020 11:55
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pfmoore pfmoore pfmoore approved these changes

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@uranusjr @pfmoore @pradyunsg
0