Support defaults via kwargs #43
Conversation
| pm.register(Plugin()) | ||
|
|
||
| # with no spec registered | ||
| assert pm.hook.myhook(arg='yeah!')[0] == "my default" |
There was a problem hiding this comment.
@nicoddemus no this is expected if you look at the code. If you want to raise an error due to missing hookspecs then use PluginManager.check_pending().
| """ | ||
| class Api: | ||
| @hookspec | ||
| def myhook(self, arg, kwarg="default"): |
There was a problem hiding this comment.
Looking at this test I kind agree with the idea of only allowing None as defaults in specs and impls. This is would be less confusing to users and doesn't prevent us to actually allow other values in the future if one of them becomes "obvious".
There was a problem hiding this comment.
@nicoddemus yes this was the consensus prior as well.
My main concern with this is we may end up with lots of if arg is None: checks throughout hookimpls. I guess it serves as an explicit way to show that your hookimpl may be further ahead then the defining project's hookspec?
goodboy
force-pushed
the
support_default_via_kwargs
branch
3 times, most recently
from
1ceb2bf to
f9f4eed
Compare
| @@ -749,10 +761,28 @@ def _maybe_apply_history(self, method): | |||
| proc(res[0]) | |||
|
|
|||
|
|
|||
| class HookSpec(object): | |||
There was a problem hiding this comment.
Even if we decide to abandon this defaults idea I really think we should keep this encapsulation of hookspec objects with a type much like we do with HookImpl. It makes introspection tasks down the road that much simpler.
|
@RonnyPfannschmidt @nicoddemus @hpk42 alright now that #42 is in this PR is a lot easier to understand and get back to. The one outstanding that everyone seemed to agree on is that if we decide to allow default arguments in @nicoddemus also clarified terminology nicely in review of #42:
def foobar(x, y, z=4):
pass
On a further note, if we decide this feature is no longer of interest I'd still like to keep a couple things from this PR that are (imho) good improvements. Namely,
I would like to get some work on #37 in before moving further with changes here but I want to get everyone thinking about it nonetheless. Thanks! |
Add support for using declared keyword argument values from both hookspecs and hookimpls. The current logic will inspect the hookimpl and, if it contains defaults, values will be first looked up from the caller provided data and if not defined will be taken from the hookspec's declared defaults. If the spec does not define defaults the value is taken from the hookimpl's defaults as is expected under normal function call semantics. Resolves pytest-dev#15
Verify that defaults declared in hook impls and specs adhere to the lookup order: call provided value, hook spec default, and finally falling back to the spec's default value.
goodboy
force-pushed
the
support_default_via_kwargs
branch
from
f9f4eed to
9fdd7f3
Compare
Allows for easier introspection of spec definitions including function signatures and hook options. Originally introduced to address pytest-dev#15 and the accompanying PR (pytest-dev#43) which requires keeping track of spec default arguments values.
Allows for easier introspection of spec definitions including function signatures and hook options. Originally introduced to address pytest-dev#15 and the accompanying PR (pytest-dev#43) which requires keeping track of spec default arguments values.
Allows for easier introspection of spec definitions including function signatures and hook options. Originally introduced to address pytest-dev#15 and the accompanying PR (pytest-dev#43) which requires keeping track of spec default arguments values.
Allows for easier introspection of spec definitions including function signatures and hook options. Originally introduced to address pytest-dev#15 and the accompanying PR (pytest-dev#43) which requires keeping track of spec default arguments values.
Allows for easier introspection of spec definitions including function signatures and hook options. Originally introduced to address pytest-dev#15 and the accompanying PR (pytest-dev#43) which requires keeping track of spec default arguments values.
Allows for easier introspection of spec definitions including function signatures and hook options. Originally introduced to address pytest-dev#15 and the accompanying PR (pytest-dev#43) which requires keeping track of spec default arguments values.
Allows for easier introspection of spec definitions including function signatures and hook options. Originally introduced to address pytest-dev#15 and the accompanying PR (pytest-dev#43) which requires keeping track of spec default arguments values.
Allows for easier introspection of spec definitions including function signatures and hook options. Originally introduced to address pytest-dev#15 and the accompanying PR (pytest-dev#43) which requires keeping track of spec default arguments values.
Allows for easier introspection of spec definitions including function signatures and hook options. Originally introduced to address pytest-dev#15 and the accompanying PR (pytest-dev#43) which requires keeping track of spec default arguments values.
Allows for easier introspection of spec definitions including function signatures and hook options. Originally introduced to address pytest-dev#15 and the accompanying PR (pytest-dev#43) which requires keeping track of spec default arguments values.
Allows for easier introspection of spec definitions including function signatures and hook options. Originally introduced to address pytest-dev#15 and the accompanying PR (pytest-dev#43) which requires keeping track of spec default arguments values.
|
@RonnyPfannschmidt I'm guess we can close this one then? |
NOTE: this PR builds upon #42 and thus contains its changes as well (for now) but will be rebased after that is merged.
This PR is an initial working implementation what was originally requested by @RonnyPfannschmidt in #15.
I still have concerns with this work (namely the reservations some have had with calling hook functions using
f(**kwargs)due to performance) because I do not believe we have properly benchmarked (looked at average versus best case perf) to discard this alternative approach. The use off(**kwargs)will greatly simplify call loop logic in_MultiCall.execute()which I think is worth acknowledging. I have created #37 to fully assess this performance question.@nicoddemus @hpk42 I realize #15 is quite lengthy but I believe this work should give us at the least some concrete code and a test to help us agree whether this feature is even something desirable considering its somewhat confusing default arguments precedence order.
As a refresher @RonnyPfannschmidt requested that
hookspeckeyword argument declarations would be used as defaults tohookimpls which require them when those arguments are not provided at hook call time. Additionally, ahookimplcan also definekwargs with default values which will be used in the case that an older version of thehookspecdefining project is being used and thus no defaults are defined on thehookspec. @hpk42 later suggested that suchkwargsshould only have a default value ofNonewhich @RonnyPfannschmidt agreed with.