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

Remove Circular dependency between Resolver and Preparer #4636

Merged
merged 14 commits into from
Oct 22, 2017

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Jul 31, 2017

Does what it says in the title.

Sidenote: GitHub went down when I was running tests and I was totally baffled by the fact that the tests were failing.

@pradyunsg pradyunsg added type: maintenance Related to Development and Maintenance Processes type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels Jul 31, 2017
@pradyunsg pradyunsg changed the title [WIP] Remove circular dependency between Resolver and Preparer Remove Circular dependency between Resolver and Preparer Aug 1, 2017
@pradyunsg pradyunsg requested a review from a team August 1, 2017 07:52
pip/resolve.py Outdated
self.require_hashes
)

# XXX: Not too sure about this. :P
Copy link
Member Author

Choose a reason for hiding this comment

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

What do I do about the following lines?

@pradyunsg pradyunsg self-assigned this Aug 2, 2017
@cjerdonek
Copy link
Member

Note to pip committers: @pradyunsg offered to wait for PR #4644 to be committed (which was originally submitted in April and has since been approved) before committing this.

@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 Aug 10, 2017
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 12, 2017
pip/resolve.py Outdated
# satisfied_by is only evaluated by calling _check_skip_installed,
# so it must be None here.
assert req.satisfied_by is None
if not self.ignore_installed:
Copy link
Member

Choose a reason for hiding this comment

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

This if-check was moved into _check_skip_installed() in my earlier PR.

pip/resolve.py Outdated
@@ -173,6 +173,59 @@ def _check_skip_installed(self, req_to_install):
self._set_req_to_reinstall(req_to_install)
return None

def _get_abstract_dist_for(self, req):
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be helpful to include a docstring stating the type of the argument and return values.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

FWIW, I'll be adding type comments to various parts of the codebase once the mypy PR merges.

pip/resolve.py Outdated
self.ignore_installed
)
if should_modify:

Copy link
Member

Choose a reason for hiding this comment

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

Delete line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! :)

pip/resolve.py Outdated
)

# NOTE
# The following portion is for determining if a certain package is
Copy link
Member

Choose a reason for hiding this comment

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

Formatting: I would do "NOTE: the following ...."

Copy link
Member Author

Choose a reason for hiding this comment

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

When I do it that way, I have an urge to indent the rest of the text to align with the colon, which feels like a waste of space. That's why I ended up doing this. :P

pip/resolve.py Outdated
# This should probably get cleaned up in a future refactor.

# req.req is only available after unpack for URL pkgs repeat
# check_if_exists to uninstall-on-upgrade (#14)
Copy link
Member

Choose a reason for hiding this comment

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

This code comment doesn't make sense / isn't a sentence?

Copy link
Member Author

@pradyunsg pradyunsg Aug 12, 2017

Choose a reason for hiding this comment

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

I copy-pasted it. I don't want to change it as a part of this PR since I'm more concerned with moving stuff than improving things.

@pradyunsg
Copy link
Member Author

Thanks for the review @cjerdonek! ^.^

@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 Sep 1, 2017
@pradyunsg
Copy link
Member Author

Ping!

@pradyunsg pradyunsg removed their assignment Oct 5, 2017
@pradyunsg pradyunsg requested review from a team and removed request for dstufft, xavfernandez and pfmoore October 6, 2017 10:58
@pradyunsg
Copy link
Member Author

Ping 2.0!

@pfmoore
Copy link
Member

pfmoore commented Oct 6, 2017

I'm not that familiar with this code, but I'll try to review it over the weekend.

@pradyunsg pradyunsg requested review from dstufft and removed request for a team October 12, 2017 16:13
@pradyunsg
Copy link
Member Author

Pinging @dstufft! :)

@pradyunsg
Copy link
Member Author

Ping @dstufft @pfmoore!

@pradyunsg
Copy link
Member Author

Note: Would conflict with #4764. :)

@pfmoore
Copy link
Member

pfmoore commented Oct 20, 2017

I didn't say which weekend ;-)

Thanks for the reminder.

@pradyunsg
Copy link
Member Author

pradyunsg commented Oct 22, 2017

Hi @pfmoore and @dstufft!

It'd be awesome if you guys could take a look at this.

A summary would be cut-paste a single function from one file to another + renaming some 4 methods in all, whose signatures change to take the values that they care about (instead of the larger Resolver object that holds the references to those objects).

@pfmoore
Copy link
Member

pfmoore commented Oct 22, 2017

I was just in the process of going to take a look when I saw the ping :-)

LGTM - I assume you'll fix up #4764 after merging this? Ping me if you need a quick review on #4764 after that, but as it'll just be mechanical changes to take care of the refactoring here, I assume it'll be pretty straightforward.

@pradyunsg
Copy link
Member Author

Thanks a lot @pfmoore!

Ping me if you need a quick review on #4764

I will. :)

@pradyunsg pradyunsg merged commit 8f68f38 into pypa:master Oct 22, 2017
@pradyunsg pradyunsg deleted the refactor/more-power-to-resolver branch October 22, 2017 17:04
@pradyunsg pradyunsg removed the type: maintenance Related to Development and Maintenance Processes label Nov 7, 2017
kianasun pushed a commit to kianasun/pip that referenced this pull request Mar 28, 2018
RequirementPreparer no longer takes Resolver as argument in any of its methods and directly takes the reference of the values it actually needs.
@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.

5 participants