Avoid cyclic dependency by merging callers.py into hooks.py #227
Conversation
The two modules have a type-level cyclic dependency: hooks.py uses _multicall and _legacymulticall from callers.py. callers.py uses HookImpl from hooks.py. This suggests that they're better off combined. The _Result functionality from callers.py is independent and general, so move it to its own module _result.py. Backward compatibility: I did not find any external project which imports directly from pluggy.callers. It exposes two potential import candidates: - HookCallError: exposed from the top-level, so no reason to reach inside. - _Result: it's prefixed and hidden, so no guarantees about that.
bluetech
force-pushed
the
merge-callers-hooks
branch
from
8d3bad6
to
36f03b2
Compare
Codecov Report
@@ Coverage Diff @@
## master #227 +/- ##
==========================================
- Coverage 93.08% 93.07% -0.02%
==========================================
Files 14 14
Lines 1677 1674 -3
Branches 117 117
==========================================
- Hits 1561 1558 -3
Misses 101 101
Partials 15 15
Continue to review full report at Codecov.
|
| return call_outcome.get_result() | ||
|
|
||
|
|
||
| class _LegacyMultiCall(object): |
There was a problem hiding this comment.
This reminds me, we can drop _LegacyMultiCall by now (#59).
There was a problem hiding this comment.
There is an open PR for this #147. Would certainly simplify things to drop it, if it's now possible.
There was a problem hiding this comment.
Yes that should land before we start moving big chunks of code around. Let's simplify first then worry about a reorg.
|
@goodboy, absolutely no rush here, but would you like for us to wait here so you can review this, or are you OK with us merging this? |
|
@nicoddemus I would like to review. One thing I definitely want to maintain in the project is clarity of authorship. Moving big chunks of code around like this can sometimes obscure that. |
There was a problem hiding this comment.
The two modules have a type-level cyclic dependency:
hooks.py uses _multicall and _legacymulticall from callers.py.
callers.py uses HookImpl from hooks.py.
This suggests that they're better off combined.
Not sure I agree that HookImpl is a cyclic dependency (caller.py never instantiates the type only uses instances) and even if it more technically is from a design perspective, moving all the caller.py code into the hooks.py module is the wrong way to solve it. The dependency should be moved out of both modules into a lower level one.
HookImpl seems to originally be a way to work around not having a proper dataclass type and should probably go into some kind of type header like module where it can be imported by multiple modules that require its interface def. We don't want to be stuck in the same situation later with some new module C that requires it and then also hooks.py requires functionality from that new module - let's hope we don't also merge C into hooks.py 😏
On top of all of this callers.py was an effort to break out the caller implementations into a separate space so we can begin to generalize the interface and eventually add new implementations soon (see #50 and #151).
The _Result functionality from callers.py is independent and general, so
move it to its own module _result.py.
This I like though I wish there was some way we could keep track of original attribution - this is @hpk42's original code. Not sure if there's a way to solve this right now.
Summary:
callers.py->_callers.py👍 (should keep attribution in git)- moving
_Resultto a lower level module 👍 (original attribution would be nice but not a blocker) - move
HookImplinto a lower level module (along with any others) to avoid cyclic interfaces.
| @@ -360,3 +361,142 @@ def __init__(self, namespace, name, opts): | |||
| self.opts = opts | |||
| self.argnames = ["__multicall__"] + list(self.argnames) | |||
| self.warn_on_impl = opts.get("warn_on_impl") | |||
|
|
|||
There was a problem hiding this comment.
Yeah so this is basically reverting work to break this stuff out of hooks.py originally (see the history). The idea here was that we want a separate package/module space so we can keep track of and add new caller implementations.
Imho putting this back into hooks is wrong and doesn't work towards that goal and we'll want to revert this again eventually anyway.
| @@ -0,0 +1,64 @@ | |||
| import sys | |||
There was a problem hiding this comment.
Yeah I'm not huge on moving big chunks like this since we lose attribution from the original author in git (unless that's been addressed in recent versions).
I am guilty of this when creating this file (which was an attempt to breakout caller implementations into a separate namespace - see comment below). I really would have liked to have kept @hpk42's original attribution here..
| return call_outcome.get_result() | ||
|
|
||
|
|
||
| class _LegacyMultiCall(object): |
There was a problem hiding this comment.
Yes that should land before we start moving big chunks of code around. Let's simplify first then worry about a reorg.
The two modules have a type-level cyclic dependency:
hooks.py uses
_multicalland_legacymulticallfrom callers.py.callers.py uses
HookImplfrom hooks.py.This suggests that they're better off combined.
The
_Resultfunctionality from callers.py is independent and general, somove it to its own module _result.py.
Backward compatibility: I did not find any external project which
imports directly from
pluggy.callers. It exposes two potential importcandidates:
HookCallError: exposed from the top-level, so no reason to reachinside.
_Result: it's prefixed and hidden, so no guarantees about that.