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

Move dependency info and install order into Resolver #4925

Conversation

pradyunsg
Copy link
Member

I couldn't split this into 2 PRs but this should be small enough too.

@pradyunsg pradyunsg added type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels Dec 16, 2017
@pradyunsg pradyunsg requested a review from dstufft December 16, 2017 14:15
@pradyunsg pradyunsg self-assigned this Dec 16, 2017
logger = logging.getLogger(__name__)


def install_given_reqs(to_install, install_options, global_options=(),
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not too happy I had to do this but I'm pretty sure that this function will move into a nicer place after a refactor of InstallRequirement, which I'll tackle after the Resolver is swapped in with the one from zazo.

@pradyunsg
Copy link
Member Author

pradyunsg commented Dec 16, 2017

It's sort of sad that only one test failed and I had to update 2 branches. This just substantially reduced the amount of confidence I have in the test suite.

note to self: note to post on close is here.

@pradyunsg pradyunsg requested a review from pfmoore January 24, 2018 10:18
@pradyunsg pradyunsg force-pushed the resolver/move-dependency-info-and-install-order branch from a622752 to 8803a31 Compare January 24, 2018 11:49
@pradyunsg
Copy link
Member Author

When merging this PR, please use plain merge instead of squash/rebase merges since I have another branch sitting on top of this one now.

@pradyunsg pradyunsg mentioned this pull request Mar 1, 2018
@pradyunsg pradyunsg added this to the 10.0 milestone Mar 1, 2018
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Mar 2, 2018
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Mar 2, 2018
@pradyunsg pradyunsg requested review from a team and removed request for pfmoore and dstufft March 4, 2018 17:11
@pfmoore
Copy link
Member

pfmoore commented Mar 26, 2018

@pradyunsg Can this be deferred? It seems to be just an internal tidy-up, and I'm not sure it's worth rushing it just to hit the beta. I've skimmed the PR and while it looks OK, I don't think I've done a thorough enough job to call it a "review" :-)

@pradyunsg
Copy link
Member Author

#5000 sits on top of this and I want that to make it through to 10.0. =)

I am happy to let you decide whether you want that to be a part of 10.0.

@pradyunsg
Copy link
Member Author

pradyunsg commented Mar 29, 2018

From #5000 and IRC, @pfmoore said he's cool with this merging w/o reviews and I'm gonna merge it now. :P

@pradyunsg pradyunsg merged commit 83cb225 into pypa:master Mar 29, 2018
@pradyunsg pradyunsg deleted the resolver/move-dependency-info-and-install-order branch March 29, 2018 11:31
@pradyunsg pradyunsg restored the resolver/move-dependency-info-and-install-order branch May 27, 2018 09:38
@pradyunsg pradyunsg deleted the resolver/move-dependency-info-and-install-order branch May 27, 2018 10:08
@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
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 skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants