Remove Circular dependency between Resolver and Preparer #4636

pradyunsg · 2017-07-31T17:20:17Z

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 · 2017-08-02T19:28:06Z

pip/resolve.py

+            self.require_hashes
+        )
+
+        # XXX: Not too sure about this. :P


What do I do about the following lines?

This reverts commit 2469633.

cjerdonek · 2017-08-08T22:42:28Z

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 · 2017-08-10T20:00:01Z

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!

cjerdonek · 2017-08-12T06:22:28Z

pip/resolve.py

+        # 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:


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

cjerdonek · 2017-08-12T06:37:53Z

pip/resolve.py

@@ -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):


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

Done.

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

cjerdonek · 2017-08-12T06:38:12Z

pip/resolve.py

+                self.ignore_installed
+            )
+            if should_modify:
+


Delete line.

Nice catch! :)

cjerdonek · 2017-08-12T06:39:04Z

pip/resolve.py

+        )
+
+        # NOTE
+        # The following portion is for determining if a certain package is


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

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

cjerdonek · 2017-08-12T06:40:26Z

pip/resolve.py

+        # 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)


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

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 · 2017-08-12T07:04:50Z

Thanks for the review @cjerdonek! ^.^

BrownTruck · 2017-09-01T00:45:00Z

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!

pradyunsg · 2017-09-27T06:59:25Z

Ping!

pradyunsg · 2017-10-06T10:59:41Z

Ping 2.0!

pfmoore · 2017-10-06T11:09:53Z

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

pradyunsg · 2017-10-14T13:04:50Z

Pinging @dstufft! :)

pradyunsg · 2017-10-20T09:44:49Z

Ping @dstufft @pfmoore!

pradyunsg · 2017-10-20T09:45:57Z

Note: Would conflict with #4764. :)

pfmoore · 2017-10-20T09:53:42Z

I didn't say which weekend ;-)

Thanks for the reminder.

pradyunsg · 2017-10-22T15:53:18Z

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 · 2017-10-22T16:56:49Z

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 · 2017-10-22T17:02:18Z

Thanks a lot @pfmoore!

Ping me if you need a quick review on #4764

I will. :)

RequirementPreparer no longer takes Resolver as argument in any of its methods and directly takes the reference of the values it actually needs.

lock · 2019-06-02T17:05:30Z

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.

pradyunsg added 4 commits July 23, 2017 22:56

Pass require_hashes explicitly

126d85f

Make methods public

b83e846

Move decision of how to prepare into pip.resolver

3862f64

🎨

2469633

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 changed the title ~~[WIP] Remove circular dependency between Resolver and Preparer~~ Remove Circular dependency between Resolver and Preparer Aug 1, 2017

pradyunsg requested a review from a team August 1, 2017 07:52

pradyunsg added 2 commits August 1, 2017 21:06

Remove of use of resolver in preparation stage

5daad29

🎨

750edae

pradyunsg commented Aug 2, 2017

View reviewed changes

pip/resolve.py Outdated

self.require_hashes

)

# XXX: Not too sure about this. :P

Copy link

Member Author

pradyunsg Aug 2, 2017

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 self-assigned this Aug 2, 2017

pradyunsg added 2 commits August 3, 2017 17:29

Revert ":art:"

924ad07

This reverts commit 2469633.

Convert comment to a useful note

ce5bd01

Merge branch 'master' into refactor/more-power-to-resolver

9c891b9

BrownTruck added the needs rebase or merge PR has conflicts with current master label Aug 10, 2017

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 12, 2017

cjerdonek reviewed Aug 12, 2017

View reviewed changes

pradyunsg added 2 commits August 12, 2017 12:49

Address review comments

831cb68

Reset the comment

33baff5

BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 1, 2017

pradyunsg requested review from xavfernandez and pfmoore September 27, 2017 06:56

pradyunsg removed their assignment Oct 5, 2017

pradyunsg requested review from a team and removed request for dstufft, xavfernandez and pfmoore October 6, 2017 10:58

pradyunsg added 2 commits October 12, 2017 21:36

Merge branch 'master' into refactor/more-power-to-resolver

efeacc7

Merge branch 'master' into refactor/more-power-to-resolver

954f17d

pradyunsg requested review from dstufft and removed request for a team October 12, 2017 16:13

pradyunsg mentioned this pull request Oct 14, 2017

pip needs a dependency resolver #988

Closed

pfmoore approved these changes Oct 22, 2017

View reviewed changes

pradyunsg merged commit 8f68f38 into pypa:master Oct 22, 2017

pradyunsg deleted the refactor/more-power-to-resolver branch October 22, 2017 17:04

pradyunsg removed the type: maintenance Related to Development and Maintenance Processes label Nov 7, 2017

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Circular dependency between Resolver and Preparer #4636

Remove Circular dependency between Resolver and Preparer #4636

pradyunsg commented Jul 31, 2017 •

edited

pradyunsg Aug 2, 2017

cjerdonek commented Aug 8, 2017

BrownTruck commented Aug 10, 2017

cjerdonek Aug 12, 2017

cjerdonek Aug 12, 2017

pradyunsg Aug 12, 2017

cjerdonek Aug 12, 2017

pradyunsg Aug 12, 2017

cjerdonek Aug 12, 2017

pradyunsg Aug 12, 2017

cjerdonek Aug 12, 2017

pradyunsg Aug 12, 2017 •

edited

pradyunsg commented Aug 12, 2017

BrownTruck commented Sep 1, 2017

pradyunsg commented Sep 27, 2017

pradyunsg commented Oct 6, 2017

pfmoore commented Oct 6, 2017

pradyunsg commented Oct 14, 2017

pradyunsg commented Oct 20, 2017

pradyunsg commented Oct 20, 2017

pfmoore commented Oct 20, 2017

pradyunsg commented Oct 22, 2017 •

edited

pfmoore commented Oct 22, 2017

pradyunsg commented Oct 22, 2017

lock bot commented Jun 2, 2019

Remove Circular dependency between Resolver and Preparer #4636

Remove Circular dependency between Resolver and Preparer #4636

Conversation

pradyunsg commented Jul 31, 2017 • edited

Choose a reason for hiding this comment

cjerdonek commented Aug 8, 2017

BrownTruck commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg Aug 12, 2017 • edited

Choose a reason for hiding this comment

pradyunsg commented Aug 12, 2017

BrownTruck commented Sep 1, 2017

pradyunsg commented Sep 27, 2017

pradyunsg commented Oct 6, 2017

pfmoore commented Oct 6, 2017

pradyunsg commented Oct 14, 2017

pradyunsg commented Oct 20, 2017

pradyunsg commented Oct 20, 2017

pfmoore commented Oct 20, 2017

pradyunsg commented Oct 22, 2017 • edited

pfmoore commented Oct 22, 2017

pradyunsg commented Oct 22, 2017

lock bot commented Jun 2, 2019

pradyunsg commented Jul 31, 2017 •

edited

pradyunsg Aug 12, 2017 •

edited

pradyunsg commented Oct 22, 2017 •

edited