Future warnings for hookspec changes AND support defaults to enable deprecation #34

goodboy · 2016-11-14T20:46:33Z

As per discussion with @RonnyPfannschmidt I'm putting this draft up to start looking at dealing with #15.

I've broke some of the tests due to a change to varnames() which now returns both the arg and kwarg names for a function.

There's also some other changes that I didn't fully revert that pertained to trying to validate kwargs that we might decide to declare in hookspecs as a way to define defaults when a hook call doesn't provide them.

RonnyPfannschmidt · 2016-11-14T20:56:49Z

pluggy.py

@@ -659,7 +688,9 @@ def varnames(func):
        return ()

    args, defaults = spec.args, spec.defaults
-    args = args[:-len(defaults)] if defaults else args
+    index = -len(defaults) if defaults else None
+    args, defaults = args[slice(0, index)], args[slice(


too fancy of an xpression

@RonnyPfannschmidt that I do it on a single line or the slice stuff?

the expression is magical enough that i have to think very hard to follow it by now
and this can be a very simple if .. else thats really easy to follow

RonnyPfannschmidt · 2016-11-14T20:58:09Z

pluggy.py

+            )
+
+        # warn about new/upcoming arguments to the spec
+        if notinimpl:


that one is explicitly breaking what we use pluggy for (as in not naming all arguments if you dont need thme all)

I still find this weird.
Is it supposed to be a feature that you don't have to match the spec?

the feature is, that a hook only has to take what it needs, no need for unused argument, and future-safety for adding new arguments

RonnyPfannschmidt · 2016-11-14T21:23:55Z

pluggy.py

+        if notinspec:
+            raise PluginValidationError(
+                "Plugin %r for hook %r\nhookimpl definition: %s\n%s"
+                "Positional args {0} are declared in the hookimpl but "


this one at least from seeing the code is more confusing than before (this may be a miss-impression)

Oh shoot I left in the format string syntax by accident...
The only thing I wanted to improve was reporting all the incompatible args in a single error instead of requiring that user runs multiple times to figure them all out.

RonnyPfannschmidt · 2016-11-14T21:24:47Z

pluggy.py

@@ -104,7 +114,7 @@ def setattr_hookspec_opts(func):
            if historic and firstresult:
                raise ValueError("cannot have a historic firstresult hook")
            setattr(func, self.project_name + "_spec",
-                   dict(firstresult=firstresult, historic=historic))
+                    dict(firstresult=firstresult, historic=historic))


can we perhaps get a separate PR fixing the indentation, its problematic to make it as part of a functionality changing pr

Yep of course.

RonnyPfannschmidt · 2016-11-14T21:25:47Z

pluggy.py


    def execute(self):
-        all_kwargs = self.kwargs
+        caller_kwargs = copy.copy(self.caller_kwargs)


at first glance this looks incomplete, was a .update call planned?

@RonnyPfannschmidt yes this was from when I was trying to approach this by calling hooks with func(**kwargs) and popping undeclared defaults beforehand.
I'll probably remove it unless we end up finding another use for it.

goodboy · 2016-11-14T22:42:47Z

@RonnyPfannschmidt bump.
Tried to fix up most of the things you asked.
The part in varnames() where I parse out the kwarg names didn't change much (still have to use the slice()). Not sure how you were envisioning I simplify it further.

RonnyPfannschmidt · 2016-11-15T08:52:29Z

pluggy.py

@@ -689,8 +677,8 @@ def varnames(func):

    args, defaults = spec.args, spec.defaults
    index = -len(defaults) if defaults else None
-    args, defaults = args[slice(0, index)], args[slice(
-        index, None if index else 0)]
+    args = args[:index]


that leads to always empty defaults due to replacing args and is till no if/else without a slice all

RonnyPfannschmidt · 2016-11-15T17:15:38Z

pluggy.py

+        index = -len(defaults)
+        args, defaults = args[:index], args[index:]
+    else:
+        defaults = []


well done
btw, shouldn't those be hash-able? (as in made into tuple)

@RonnyPfannschmidt we do hash them per function already in varnames() no?

ohhh you mean each list of names. Yeah I can add that.

goodboy · 2016-11-18T00:05:21Z

@RonnyPfannschmidt @hpk42 bump!

I think this adds what you originally requested @RonnyPfannschmidt but I personally think it's quite hideous, ill performs, and is confusing given normal function call semantics.

I still have to add tests of course.

Warn when either a hook call doesn't match a hookspec. Additionally, - Extend `varnames()` to return the list of both the arg and kwarg names for a function - Rename `_MultiCall.kwargs` to `caller_kwargs` to be more explicit - Store hookspec kwargs on `_HookRelay` and `HookImpl` instances Relates to pytest-dev#15

goodboy · 2016-11-18T16:24:50Z

@RonnyPfannschmidt @hpk42 rebased onto the new test reorg changes.

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 · 2016-11-19T17:53:03Z

@RonnyPfannschmidt @hpk42 alright I added a test which shows how this all works now.

I still think the implementation of _MultiCall.execute() would be better implemented like I suggested quite a while ago using func(**kwargs).

I'd like you guys to take a look before I do any more changes since I'm pretty sure this PR's current state matches the original requirements of #15.

goodboy · 2016-11-19T17:57:29Z

pluggy.py

+        self.kwargvalues = inspect.getargspec(function).defaults
+        self.kwargs = dict(
+            ((name, value) for name, value in
+             zip(self.kwargnames, inspect.getargspec(function).defaults))


inspect.getargspec(function).defaults should just be self.kwargvalues here?

nicoddemus · 2017-01-31T23:39:41Z

Just a gentle ping, what's the status of this PR?

goodboy · 2017-02-01T01:45:18Z

@nicoddemus well it's in dire need of thorough review :)

I have my own criticisms of this particular approach but I wanted to at least get something going for #15.

I know @RonnyPfannschmidt mentioned that he thought a test might be missing.

goodboy · 2017-02-09T05:49:53Z

pluggy.py

@@ -724,6 +745,8 @@ def remove(wrappers):
                raise ValueError("plugin %r not found" % (plugin,))

    def _add_hookimpl(self, hookimpl):
+        """A an implementation to the callback chain.


goodboy · 2017-02-13T01:30:56Z

Declining this as I have reintroduced the change set using two smaller (and more focused) PRs #42 and #43 to simplify the review process.

RonnyPfannschmidt reviewed Nov 14, 2016

View reviewed changes

RonnyPfannschmidt reviewed Nov 15, 2016

View reviewed changes

goodboy mentioned this pull request Nov 15, 2016

support defaults to enable deprecation #15

Closed

goodboy force-pushed the master branch 3 times, most recently from b61f9a2 to 9ab0ebe Compare November 15, 2016 23:37

Tyler Goodlet added 2 commits November 18, 2016 10:53

Port tests to match new varnames() return value

22f3b0a

goodboy force-pushed the master branch from dd4641e to 0fc2930 Compare November 18, 2016 16:24

Tyler Goodlet added 2 commits November 19, 2016 12:46

Add a defaults precedence test

178ffdf

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 master branch from 0fc2930 to 178ffdf Compare November 19, 2016 17:49

goodboy commented Nov 19, 2016

View reviewed changes

goodboy commented Feb 9, 2017

View reviewed changes

goodboy changed the title ~~Future warnings for hookspec changes~~ Future warnings for hookspec changes AND support defaults to enable deprecation Feb 9, 2017

goodboy mentioned this pull request Feb 13, 2017

Hook call mismatch warnings #42

Merged

goodboy closed this Feb 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Future warnings for hookspec changes AND support defaults to enable deprecation #34

Future warnings for hookspec changes AND support defaults to enable deprecation #34

goodboy commented Nov 14, 2016

RonnyPfannschmidt Nov 14, 2016

goodboy Nov 14, 2016

RonnyPfannschmidt Nov 14, 2016

RonnyPfannschmidt Nov 14, 2016

goodboy Nov 14, 2016

RonnyPfannschmidt Nov 14, 2016

RonnyPfannschmidt Nov 14, 2016

goodboy Nov 14, 2016

RonnyPfannschmidt Nov 14, 2016

goodboy Nov 14, 2016

RonnyPfannschmidt Nov 14, 2016

goodboy Nov 14, 2016

goodboy commented Nov 14, 2016

RonnyPfannschmidt Nov 15, 2016

RonnyPfannschmidt Nov 15, 2016

goodboy Nov 15, 2016

goodboy Nov 15, 2016

goodboy commented Nov 18, 2016

goodboy commented Nov 18, 2016

goodboy commented Nov 19, 2016

goodboy Nov 19, 2016

nicoddemus commented Jan 31, 2017

goodboy commented Feb 1, 2017

goodboy Feb 9, 2017

goodboy commented Feb 13, 2017

Future warnings for hookspec changes AND support defaults to enable deprecation #34

Future warnings for hookspec changes AND support defaults to enable deprecation #34

Conversation

goodboy commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodboy commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodboy commented Nov 18, 2016

goodboy commented Nov 18, 2016

goodboy commented Nov 19, 2016

Choose a reason for hiding this comment

nicoddemus commented Jan 31, 2017

goodboy commented Feb 1, 2017

Choose a reason for hiding this comment

goodboy commented Feb 13, 2017