Hook call mismatch warnings #42

goodboy · 2017-02-13T00:43:03Z

This add's proper warnings for hook calls which do not match the corresponding hookspec's signature. The idea is to encourage callers of hooks to always keep up to date with the most recent spec.

This comes from discussion in #15.

@hpk42 @nicoddemus @RonnyPfannschmidt this is the first part of #34 which I have separated out to simplify the review and narrow focus.

goodboy · 2017-02-14T23:37:34Z

@hpk42 @nicoddemus @RonnyPfannschmidt any opinions on this?
I was hoping to move along sooner then later ;)

nicoddemus

Thanks @tgoodlet for your work!

nicoddemus · 2017-02-15T00:25:12Z

pluggy.py

@@ -581,7 +595,7 @@ def varnames(func):
        try:
            func = func.__init__
        except AttributeError:
-            return ()
+            return (), ()


We should update the docs for this function given the new return value.

i just noticed something dreadful, since this is a public name, this is a breaking change would warrant a major incompatible release

as an example, if we had used this in pytest and didn't do vendoring, this would be a release that breaks pytest

@RonnyPfannschmidt you got a point, we should increase the version to 0.5.0 and mention this change in the CHANGELOG.

In addition, should we make this method private and remove it from the public API?

Afaik this isn't "really" part of the public API due to the contents of pluggy.all?

Oh that's a good point.

tox doesn't use it as well, neither does devpi using a local search (BB doesn't seem to have a search feature).

I think we can safely rename it to an internal function to avoid confusion then. IMHO no need to provide a wrapper for it since nobody outside pluggy uses it as far as we know, and in case someone complains and makes a good case for it we can reintroduce it and promote to the public API.

0.X is the period of time we can break things if we think that's better. 😉

@RonnyPfannschmidt if you agree then I won't bother

i do agree on practicality vs purity grounds

@RonnyPfannschmidt so do you still want the wrapper or no?

@tgoodlet its not needed, i meant that i agree with @nicoddemus that on grounds of probably controlled set of users and pre 1.0 we can just make it private without going the extra mile

nicoddemus · 2017-02-15T00:27:19Z

pluggy.py

@@ -605,10 +623,10 @@ def varnames(func):

    assert "self" not in args  # best naming practises check?
    try:
-        cache["_varnames"] = args
+        cache["_varnames"] = args, defaults
    except TypeError:


I know this code was here before already, but I'm curious in which situations can cache["_varnames"] raise a TypeError.

@nicoddemus yeah I never really understood the weird "storing arg names in the function's dict" in the first place. Can't we just have a single global cache which hashes by object id?

Maybe @hpk42 can comment?

@tgoodlet i remember that this optimization safes a few hops, and going for a cache on the object is very practical as it indeed ties object lifetime and cache lifetime

@RonnyPfannschmidt huh that's an interesting point.

I've always understood that function lifetimes last from module import until interpreter exit unless you explicitly delete references to them. Also consider varnames() is only normally called once at PluginManager.register() time (really only once for each hookspec or hookimpl) so I might even go so far as to say this whole cache idea is a pre-mature optimization.

That being said @hpk42 had it there originally and I see no strong reason to remove it.

Oh and if we did end up wanting to use a global cache we could just use weakref.WeakValueDictionary and get the same gc behavior no?

@nicoddemus btw do you think this is worth making a separate issue?

Perhaps writing a series of banchmarks so we could have a safe suite to test optimizations would be something worth exploring.

@nicoddemus agreed that's what #37 is for ;)

Maybe I'll hack on that next in an effort to rally some faith!

@tgoodlet for an weak dictionary there is a need to handle some things differently, unless im mistaken there will be a need to decompose to method objects, and it will create a new magical global managing lifetimes, personally i believe its much more safe to bind lifetimes to the objects directly

@RonnyPfannschmidt I'm not sure it will matter but maybe there's something I'm missing. I do think we should leave it for now.

If you think I should open discussion about then I'll make a separate issue :)

nicoddemus · 2017-02-15T00:40:10Z

pluggy.py

+        notinspec = set(hookimpl.argnames) - set(hook.argnames)
+        if notinspec:
+            raise PluginValidationError(
+                "Plugin %r for hook %r\nhookimpl definition: %s\n"


I was under the impression we should always use the term "arguments" or "keyword arguments" when conveying information to users, given that for all purposes hookimpls should always match the argument names of hookspecs, but position is not of importance. At least that's how pluggy and pytest hooks work in my mind.

@nicoddemus the only thing is if we're going to support actual keyword arguments as in #43, then shouldn't we distinguish?

Isn't #43 about supporting default values?

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

As far as I understand, hooks are always called using keyword arguments so order is not an issue, that's why my comment was referring to "Positional arguments" as confusing.

Note that I'm talking about the caller here, not the definition. The definition might for example force the caller to call a function using only keywords:

def foobar(*, x, y, z=4): pass

(Python 3 only of course)

AFAIU, pluggy always calls using keyword arguments, so specs are implicitly defined as:

def pytest_deselectitems(*, items, session): pass

And that issue is completely separated from default values.

Am I missing something?

@nicoddemus no you're definitions are absolutely correct.
Sorry when I said "keyword" I meant to say "default".

So yes my concern is about being explicit with regard to hookimpl to hookspec mismatches regarding default arguments which is really not in the scope of this PR (it's coupled here because originally #43 and this were one PR).

I'll revert and thanks for clarifying.

Oh OK that explains it. 😄

Thanks!

@nicoddemus alright fixed. Let me know if there's anything else otherwise I'll squash the history.

nicoddemus · 2017-02-15T00:40:44Z

pluggy.py

+        notincall = set(self.argnames) - set(kwargs.keys())
+        if notincall:
+            warnings.warn(
+                "Positional arg(s) %s are declared in the hookspec "


Same comment about "Positional arg" I made in my previous comment.

This one I agree with since we only allow calling with "named argument" syntax currently.

nicoddemus · 2017-02-15T00:41:49Z

testing/test_helpers.py

@@ -13,16 +13,16 @@ class B(object):
        def __call__(self, z):


Would it be possible to test the generated warnings?

Yes good call 👍

goodboy · 2017-02-16T07:21:41Z

@nicoddemus fixed up most of it and added the test. Let me know what you think.

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

- don't use "Positional" in warning message - support python 2.6 sets - don't include __multicall__ in args comparison - document `varnames()` new pair return value

goodboy · 2017-02-18T15:47:20Z

@hpk42 @nicoddemus rebased after #46.

I think this should be ready. Is there any more issues you guys want me to sort out?

@hpk42 you've been a bit silent :(

goodboy · 2017-02-22T20:11:28Z

@nicoddemus @hpk42 anything else you guys aren't happy with?

nicoddemus

LGTM, but I think @hpk42 or @RonnyPfannschmidt should merge it as they know pluggy in more depth than me.

RonnyPfannschmidt · 2017-02-24T14:22:28Z

@nicoddemus i believe this one can be merged, but we will absolutely have to put that wrapper/rename in place before the next release

@tgoodlet if you get that in before @nicoddemus chimes in i can also just merge

in any case good work and thanks for staying on top of this "never-ending story" 👍

goodboy · 2017-02-24T19:06:16Z

@RonnyPfannschmidt yes I'll get the wrapper in today 👍
Can you also look at some of my other responses ^ I just want to make sure we're on the same page :)

And thanks for the thanks!

goodboy · 2017-02-25T00:47:29Z

@RonnyPfannschmidt @nicoddemus ok I'll squash the history and merge this once I get back later tonight if you guys both give the thumbs up!

nicoddemus · 2017-02-25T01:05:36Z

👍 please go ahead!

nicoddemus · 2017-02-25T03:19:43Z

Squashed and merged. 👍

Thanks again @tgoodlet, excellent work!

goodboy · 2017-02-25T06:54:28Z

@nicoddemus my pleasure :)

goodboy · 2017-02-25T17:05:18Z

@nicoddemus huh weird how that doesn't look like a merge in the git history?
Almost looks like it was put directly on master?

nicoddemus · 2017-03-03T12:49:41Z

I did use the "Squash and merge" option in GH's UI, I thought that's what we wanted since there were quite a few interim commits in this PR... if we wanted a normal merge I apologize.

RonnyPfannschmidt · 2017-03-03T12:55:43Z

@nicoddemus i think we should avoid squashes in future, my experience with them has been surprising and frustrating as well

nicoddemus · 2017-03-03T12:58:01Z

No problem @RonnyPfannschmidt, my mistake!

goodboy · 2017-03-03T20:19:10Z

@nicoddemus @RonnyPfannschmidt yeah I when I said "squashed" I practically meant I would git rebase -i into less commits; not a literal GH "squash".

I can break this history up into the two commits I had in mind (impl and test) if you guys don't me rewriting history on master? It shouldn't affect anyone given there was no release made and I'm the only one hacking atm.

RonnyPfannschmidt · 2017-03-03T20:24:55Z

hmm, never ever rewrite history on master ^^ we can undo the pr and accept a new one, but rewriting master is a absolute no go

nicoddemus · 2017-03-14T12:43:33Z

never ever rewrite history on master

I agree... let's just accept this as a mistake and move on. 😁

This was referenced Feb 13, 2017

Support defaults via kwargs #43

Open

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

Closed

nicoddemus reviewed Feb 15, 2017

View reviewed changes

Tyler Goodlet added 4 commits February 18, 2017 10:44

Port tests to match new varnames() return value

e27338e

Handle py2.6 and better docs

96b6494

- don't use "Positional" in warning message - support python 2.6 sets - don't include __multicall__ in args comparison - document `varnames()` new pair return value

Add a call vs. spec mismatch warning test

4ab6b1f

goodboy force-pushed the hook_call_mismatch_warnings branch from 61b3884 to 4ab6b1f Compare February 18, 2017 15:46

Drop "Positional" adjective

e5cfb31

nicoddemus approved these changes Feb 24, 2017

View reviewed changes

nicoddemus merged commit 26ddfe4 into pytest-dev:master Feb 25, 2017

edmorley mentioned this pull request May 26, 2017

Update pytest to 3.1.0 mozilla/treeherder#2497

Closed

pyup-bot mentioned this pull request Sep 6, 2017

Update pluggy to 0.5.2 amureki/whoisp#47

Closed

pyup-bot mentioned this pull request Sep 18, 2017

Scheduled weekly dependency update for week 38 qutebrowser/qutebrowser#2985

Closed

pyup-bot mentioned this pull request Sep 25, 2017

Scheduled weekly dependency update for week 39 qutebrowser/qutebrowser#3031

Merged

pyup-bot mentioned this pull request Oct 5, 2017

Initial Update aio-libs/async-lru#11

Closed

pyup-bot mentioned this pull request Oct 29, 2017

Update pluggy to 0.5.2 uppsaladatavetare/foobar-api#56

Closed

This was referenced Nov 24, 2017

Update pluggy to 0.6.0 amureki/whoisp#76

Closed

Update pluggy to 0.6.0 DavidHickman/calendary#181

Open

Update pluggy to 0.6.0 StuartJSquires/markov_slackbot#147

Closed

pyup-bot mentioned this pull request Jan 19, 2018

Update pluggy to 0.6.0 Poorvak/http_pipeline#8

Merged

pyup-bot mentioned this pull request Feb 8, 2018

Initial Update aio-libs/aiocassandra#9

Merged

Hook call mismatch warnings #42

Hook call mismatch warnings #42

Conversation

goodboy commented Feb 13, 2017

goodboy commented Feb 14, 2017

nicoddemus left a 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

nicoddemus Feb 24, 2017 • edited Loading

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 Feb 16, 2017 • edited Loading

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

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 Feb 16, 2017

goodboy commented Feb 18, 2017

goodboy commented Feb 22, 2017

nicoddemus left a comment

Choose a reason for hiding this comment

RonnyPfannschmidt commented Feb 24, 2017

goodboy commented Feb 24, 2017

goodboy commented Feb 25, 2017

nicoddemus commented Feb 25, 2017

nicoddemus commented Feb 25, 2017

goodboy commented Feb 25, 2017

goodboy commented Feb 25, 2017

nicoddemus commented Mar 3, 2017

RonnyPfannschmidt commented Mar 3, 2017

nicoddemus commented Mar 3, 2017

goodboy commented Mar 3, 2017

RonnyPfannschmidt commented Mar 3, 2017

nicoddemus commented Mar 14, 2017

nicoddemus Feb 24, 2017 •

edited

Loading

goodboy Feb 16, 2017 •

edited

Loading