pluggy
pluggy copied to clipboard
Support defaults via kwargs
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 of f(**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 hookspec keyword argument declarations would be used as defaults to hookimpls which require them when those arguments are not provided at hook call time. Additionally, a hookimpl can also define kwargs with default values which will be used in the case that an older version of the hookspec defining project is being used and thus no defaults are defined on the hookspec. @hpk42 later suggested that such kwargs should only have a default value of None which @RonnyPfannschmidt agreed with.
@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 hookspec and hookimpl then they should only be allowed to have a None default value.
@nicoddemus also clarified terminology nicely in review of #42:
Let's see if we are using the same nomenclature first. Considering this function:
def foobar(x, y, z=4):
pass
- positional arguments: caller passes by position so order matters: foobar(1, 2, 10)
- keyword arguments: caller passes using keyword syntax so order does not matter: foobar(y=2, x=1, z=10)
- default arguments: caller does not need to pass them on: foobar(1, 2) # z = 4
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,
- encapsulating
hookspecobjects using aHookSpectype- makes future introspection tasks much easier
- avoid monkey patching (eg.
_HookCaller._specmodule_or_class)- easier to understand code
- make
_MultiCallas hook agnostic as possible- maintain a separation of concerns
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!
@RonnyPfannschmidt I'm guess we can close this one then?