Reduce action at distance #4978
Conversation
pradyunsg
added
skip news
pradyunsg
force-pushed
the
refactor/reduce-action-at-distance
branch
from
5a81ef5 to
146c390
Compare
|
A nice clean history here now. =) |
|
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. |
|
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 |
|
Ohkay... This is interesting:
Py3.5 job timed out, not PyPy or PyPy3. :| @pfmoore any ideas why? It seems to be a simple timeout. |
|
@pradyunsg I'm not going to have time to do a proper review of this before the beta. On a superficial look, I'm not sure it improves clarity (the manual setting of If you feel it's worth merging, I won't object, but maybe it can wait? There's no user-facing impact either way. |
| @@ -81,10 +76,11 @@ def add_requirement(self, install_req, parent_req_name=None, | |||
| wheel.filename | |||
| ) | |||
|
|
|||
| install_req.use_user_site = self.use_user_site | |||
There was a problem hiding this comment.
@pfmoore the intention is to have requirementset.add do less. By moving this outside, I've tried to make it explicit which paths go which way.
This makes it easier to keep track of what's happening (because this bit of code is highly intertwined). I'm sort of trying to untangle this bit of code here.
There was a problem hiding this comment.
Yeah, I think I need to see the whole file - github's review interface isn't that great for changes like this.
OTOH, if the tests pass, and you're happy that the code is easier to maintain, I don't really have a problem with it going in. Feel free to merge it without me going through it in detail, if you want.
| self.require_hashes, | ||
| self.finder, | ||
| req, self.require_hashes, self.use_user_site, self.finder, | ||
|
|
There was a problem hiding this comment.
Remove this empty line.
|
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 |
|
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
bot
added
the
auto-locked
Removing lines 84-87 in req_set was the intent here, eliminating some not-so-nice action at a distance behaviour, which should make it bit easier to reason about in this part of the codebase.