support defaults to enable deprecation

the idea is, that if a hook is invoked with a missing, but specced argument, then the default is used instead

so if a hook caller would call hook.pytest_deselected(items=...)' and the hook was defined withdef pytest_deselected(items, reason='No Reason Given)`
then the receivers would receive the defined default reason

in order to support deprecation of such usage we could later devise a mark for deprecated defaults

Usually a project defining a hookspec is also calling it. So this is basically a very minor convenience way of expressing a default and it blurs the line between what is a spec and what is implementation. I think it's clearer to request from a hook calling site to provide all arguments and not have to worry cross-checking with the spec on what parameters are actually passed to impls.

@hpk

@hpk - this is directly broken for how we document pytest_deselected

atm its missing a actually sane way to transfer the reason

What exactly is broken? What is not working as expected? At the moment we don't promise that we do anything with default arguments in specs. They are currently ignored -- we could throw an error to be more explicit. I hold providing call-kwargs values through a hookspec is confusing.

@hpk42

@hpk42 the underlying problem is the following

pytest_deselected(items) is documented to be called as hook

if we now want to add a reason argument so it becomes pytest_deselected(items, reason) - all external users break

ah, now i get it, thanks. So this is a situation where callers of a hook live in separate projects and thus changing the spec becomes hard. This kind of trumps (!) my concern then i guess. Feel free to submit a PR. if you hurry it might make it into pluggy-0.5.

@hpk42

@hpk42 pretty sure I can nail out this one :)

@hpk42

On Fri, Nov 11, 2016 at 07:04 -0800, goodboy wrote:

@hpk42 pretty sure I can nail out this one :)

@nicoddemus reminded me that pytest actually has its own vendored version of pluggy. If that is true also for older pytest versions (i think it is) we don't actually need to support multicall neccessarily anymore. It might mean, though, that pytest only wants to go for vendoring a non-multicall pluggy for pytest-4.0. Which in turns means we might want to support it for a while still at best through an opt-in parameter to instantiating a PluginManager.

@tgoodlet this one might be a bit tricky

@RonnyPfannschmidt

@RonnyPfannschmidt just to make sure I understand this correctly:

hookspec = HookspecMarker("prefix")
hookimpl = HookimplMarker("prefix")

class Spec:
    @hookspec
    def prefix_myhook(self, doggy, kitty='mean'):
        pass

@hookimpl
def prefix_myhook(doggy, kitty):
    assert doggy is not 'smelly', "Kick out the dog"
    print("shocker kitty is always {}".format(kitty))


@hookimpl
def prefix_myhook(doggy, kitty='smelly'):
    assert doggy is not 'smelly', "Kick out the dog"
    print("shocker kitty is always {}".format(kitty))

pm = PluginManger('prefix')
pm.add_hookspec(Spec)
pm.register(sys.modules[__name__])

precedence is given to the spec's default arg unless overridden in the implementation
passing the arg explicitly in the hook call overrides both?

>>> pm.hook.prefix_myhook(doggy='just bathed')
"shocker kitty is always mean"  # from the first impl
"shocker kitty is always smelly"  # from the second impl

>>> pm.hook.prefix_myhook(doggy='just bathed', kitty='grumpy')
"shocker kitty is always grumpy"  # from the first impl
"shocker kitty is always grumpy"  # from the second impl

uh, that precedence questions highlights that allowing default args in specs is tricky. if anything, i'd argue that the impl's default should take precedence over the spec's default.

I am afraid however we decide about precedence, it's going to be surprising to some and you have to check three places to understand where a value comes from ... sorry, ronny, i am getting unsure again if this is the right approach to solve the underlying problem we discussed earlier: callers of a hook become incompatible if an argument is added to the spec. I'd now like to get others to comment on the problem before you tackle impl, tests, docs.

My view is that pluggy is foremost about 1:N calls, not sure much about M:N calls (with M = numb of the different places where a call is made).

On a side note, maybe this pytest deselect hook calling is a bit of a bad idea anyway. pytest's core collection hook should automatically call it by looking at pre-post list of items. Or it should even all be just done by the terminal reporter. This way user code is not concerned with it at all.

@RonnyPfannschmidt

uh, that precedence questions highlights that allowing default args in specs is tricky. if anything, i'd argue that the impl's default should take precedence over the spec's default.

My initial reaction was the opposite, which only emphasizes your point about this being tricky.

maybe this pytest deselect hook calling is a bit of a bad idea anyway.

FWIW, I always thought it a bit odd... it felt weird calling a core hook from a plugin implementation.

Or it should even all be just done by the terminal reporter.

The same thought occurred to me when I had to call pytest_deselected in a plugin once. I thought at the time the terminal writer could find out which tests were deselected automatically by writing a hookwrapper around pytest_collection_modifyitems.

Do we have other hooks with the same behavior as pytest_deselected(items)? Perhaps @RonnyPfannschmidt has other examples from custom plugins that have the same behavior.

If there's no other cases, we might instead consider just getting rid of the odd child that is pytest_deselected.

@hpk42

I am afraid however we decide about precedence, it's going to be surprising to some and you have to check three places to understand where a value comes from

@hpk42 just want to clarify the 3 places in reverse precedence order - spec, impl, call.

I guess your concern is with debugging? As far as I understand the implementer of a hook shouldn't be too concerned about where a value came from.

callers of a hook become incompatible if an argument is added to the spec.

@hpk42 I'm not sure this is true as long as the new argument is a kwarg. In fact that's usually the beauty of one.

As an example I already have a working draft patch of _MultiCall.execute() which looks as follows:

     def execute(self):
-        all_kwargs = self.kwargs
+        all_kwargs = copy.copy(self.kwargs)
         self.results = results = []
         firstresult = self.specopts.get("firstresult")

         while self.hook_impls:
             hook_impl = self.hook_impls.pop()
+            if "__multicall__" in hook_impl.argnames:
+                all_kwargs["__multicall__"] = self
             try:
-                args = [all_kwargs[argname] for argname in hook_impl.argnames]
+                args = [all_kwargs.pop(argname) for argname in hook_impl.argnames]
             except KeyError:
                 for argname in hook_impl.argnames:
-                    if argname not in all_kwargs:
+                    if argname not in self.kwargs:
                         raise HookCallError(
                             "hook call must provide argument %r" % (argname,))
             if hook_impl.hookwrapper:
-                return _wrapped_call(hook_impl.function(*args), self.execute)
-            res = hook_impl.function(*args)
+                return _wrapped_call(hook_impl.function(*args, **all_kwargs),
+                                     self.execute)
+            res = hook_impl.function(*args, **all_kwargs)
             if res is not None:
                 if firstresult:
                     return res

My view is that pluggy is foremost about 1:N calls, not sure much about M:N calls (with M = numb of the different places where a call is made).

Fwiw M calls turns out to be very useful for event like systems made with pluggy. I'm actually using this for a project where we signal to plugins the dynamic setup/teardown of network resources. This allows plugins to do things like log capture and tracing on remote hosts that are provided by elastic VM and container systems.

@hpk42

my current oppinion is that spec defaults should always win over impl defaults

a hookimpl should be able to declare the default for backward compatibility (say a hook adds a new parameter you want to use if availiable, but still support older versions of the program that did not support/supply it

@hpk42 the items object for the modifyitems hook could have a richer api than just being a normal list
one thing is certain, terminalwriter is unable to figure a reason ^^

Add defaults support to enable deprecation

Encapsulate spec definitions with a class

closing this one, i decided new hook names should be the way to go

Encapsulate spec definitions with a class

Navigation Menu

Repository navigation

support defaults to enable deprecation #15

49 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

support defaults to enable deprecation #15

Description

Activity

hpk42 commented on Nov 11, 2016

RonnyPfannschmidt commented on Nov 11, 2016

hpk42 commented on Nov 11, 2016

RonnyPfannschmidt commented on Nov 11, 2016

hpk42 commented on Nov 11, 2016

goodboy commented on Nov 11, 2016

hpk42 commented on Nov 11, 2016

RonnyPfannschmidt commented on Nov 11, 2016

goodboy commented on Nov 12, 2016

hpk42 commented on Nov 12, 2016

nicoddemus commented on Nov 12, 2016

goodboy commented on Nov 12, 2016

RonnyPfannschmidt commented on Nov 12, 2016

49 remaining items

RonnyPfannschmidt commented on Aug 16, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions