add specname option to hookimpl #251

tlambert03 · 2020-01-27T00:30:11Z

This PR adds a specname option to @hookimpl. If specname is provided, it will be used instead of the function name when matching this hook implementation to a hook specification during registration (allowing a plugin to register a hook implementation that was not named the same thing as the corresponding hookspec).
(Edited:) This is related to #218 (though likely different than that poster’s intentions)

Most of the interesting discussion is happening in #218 (@RonnyPfannschmidt voiced some opposition to the idea at one point), and you can see my use-case/rationale for wanting this feature in #218 (comment). But I figured since it's a small change, I would submit this so you could see exactly what I was proposing to make the discussion more concrete. Curious to hear your thoughts.

EDIT:
I should add that I'm not 100% sure this addresses the original poster's request in #218. They wanted a many-to-one registration, I simply want registration of non-matching function names.

goodboy · 2020-01-27T03:06:15Z

@tlambert03 I will take a look at this week hopefully.
Hopefully some others will pipe in with opinions as well.

Thanks for the PR too 👍

asottile

implementation seems fine
tests need a few small tweaks
I'm not sure I know enough what this would enable / break, but seems mostly harmless to me 👍

testing/test_hookcaller.py

RonnyPfannschmidt · 2020-01-27T06:16:25Z

This does not solve #218, as hooks are dealt, not hookspecs

tlambert03 · 2020-01-27T09:17:15Z

does not solve #218

Yes, as mentioned in my comment above, I realize that though the syntax is the same as the original poster’s request in #218, the goal is likely not the same. So I will remove the “fixes” flag and you can keep that issue open if you’d like. However, this achieves what I was requesting, so do you have any thoughts/comments on the PR as it stands on its own, outside of the context of #218?

RonnyPfannschmidt · 2020-01-27T11:20:18Z

its a nice addition, similar to fixture(name="foo") the hookimpl(name="foo")

whereas #218 was looking for something like hookspec(name=re.compile("foo_(P<pluginname>.*)")

tlambert03 · 2020-01-27T12:13:19Z

Thanks @RonnyPfannschmidt! I will address the tests requests from @asottile and also curious to hear what @goodboy thinks.

nicoddemus

Other than the comments made by @asottile, this looks good to me.

asottile

tlambert03 · 2020-02-04T21:34:07Z

hi @goodboy, any thoughts on this PR?

one final question might be whether everyone is ok with the argument being named specname instead of simply name or hookname... let me know if you have thoughts on that.

not directly related: we've decided to use pluggy as our plugin manager over at napari and we're all excited about it :)

goodboy · 2020-02-04T21:36:16Z

@tlambert03 so sorry. I'm ashamed the level of attention has been diverted away from pluggy over the past year..
I will do my best to look now.

we've decided to use pluggy as our plugin manager over at napari and we're all excited about it :)

Oh, amazing. Hopefully we can make some good progress then moving forward 😸

tlambert03 · 2020-02-04T21:37:37Z

not a problem! I know how these things go with open source. thanks for your time!

goodboy

@tlambert03 this looks pretty good!

I left a few minor comments that I'd like to hear your opinion on.

One other outstanding would be to document or make an issue that we need to document this feature in an upcoming release.

Thanks again for the work!
It's nice to see some more eyes on the code base 🏄‍♂️

goodboy · 2020-02-05T04:20:28Z

testing/test_hookcaller.py

+    def he_myhook2(arg1):
+        pass
+
+    assert he_myhook1.example_impl.get("specname") is None


Hmm not that it's a big deal but if hooks.py:130 was removed this check would still pass.
Not sure if we need to be strict about the "normalization" entry in the opts dict here.

yeah good point. that's probably an unnecessary test, since test_hookimpl already pretty much makes sure that hookimpl parameters get added. Was just trying to follow the pattern. Should I just remove it?

Yeah I'm not sure. If it's redundant I guess we don't need it?

src/pluggy/manager.py

testing/test_hookcaller.py

tlambert03 · 2020-02-05T14:06:35Z

One other outstanding would be to document or make an issue that we need to document this feature in an upcoming release.

also forgot to mention: I already created a PR #252 specifically for this. Couple things to discuss over there, but it's mostly set up.

goodboy · 2020-02-05T14:08:13Z

Hmm also code coverage target was missed eh?
I wonder if that was because of the redundant check you removed?

tlambert03 · 2020-02-05T14:10:06Z

Hmm also code coverage target was missed eh?
I wonder if that was because of the redundant check you removed?

yeah, I'm looking at the codecov report now... I'm confused as well :/ relative to master, coverage should be the same if not slightly higher?

testing/test_hookcaller.py

goodboy

Yeah looks great.

Only last thing would be pleasing the coverage target.

tlambert03 · 2020-02-05T18:10:23Z

well, I guess it's working now. I did effectively nothing but trigger another push. I think it must have been comparing coverage to the previous commit and not to master? anyway. Thanks for the review. This looks ready to go. See you over in #252 ...

tlambert03 · 2020-02-05T19:35:53Z

thanks!

Looks like pytest-dev#251 was forgotten in the changes for 1.0.0?

Include #251 in the release notes for 1.0.0

Refs pytest-dev/pluggy#251 Refs simonw/datasette#1575

simonw · 2021-12-20T01:47:27Z

This also means that a single Python module can now register more than one implementation for the same hook, which is useful. https://til.simonwillison.net/pluggy/multiple-hooks-same-file

tlambert03 added 3 commits January 26, 2020 19:24

add specname option to hookimpl

9fddd54

typo

372fc92

add newline

a568a9f

tlambert03 mentioned this pull request Jan 27, 2020

Implement pluggy as plugin manager napari/napari#908

Merged

fix black double-quotes

a3b11f7

goodboy requested review from nicoddemus, asottile and RonnyPfannschmidt January 27, 2020 03:06

asottile reviewed Jan 27, 2020

View reviewed changes

testing/test_hookcaller.py Show resolved Hide resolved

testing/test_hookcaller.py Outdated Show resolved Hide resolved

RonnyPfannschmidt approved these changes Jan 27, 2020

View reviewed changes

nicoddemus approved these changes Jan 27, 2020

View reviewed changes

fix tests

a10dbae

asottile approved these changes Jan 27, 2020

View reviewed changes

This was referenced Jan 27, 2020

add docs for specname #252

Merged

Plans for plugin PRs napari/napari#936

Closed

tlambert03 mentioned this pull request Feb 3, 2020

Sphinx docs restructuring (create space for dev-focused prose) napari/napari#942

Merged

goodboy reviewed Feb 5, 2020

View reviewed changes

tlambert03 added 4 commits February 5, 2020 07:39

remove unnecessary test

3066398

return local specname var to name

64ec41f

assert matching signature with specname option

a57f279

add check_pending test for specname

ef768f2

goodboy reviewed Feb 5, 2020

View reviewed changes

testing/test_hookcaller.py Outdated Show resolved Hide resolved

goodboy approved these changes Feb 5, 2020

View reviewed changes

tlambert03 added 2 commits February 5, 2020 12:34

tweak tests

dbb1c44

trailing whitespace

e37c8d0

goodboy merged commit 6e8b6d4 into pytest-dev:master Feb 5, 2020

tlambert03 deleted the hookimpl-specname branch February 5, 2020 19:28

cjolowicz added a commit to cjolowicz/pluggy that referenced this pull request Sep 1, 2021

Include pytest-dev#251 in the release notes for 1.0.0

ec55412

Looks like pytest-dev#251 was forgotten in the changes for 1.0.0?

cjolowicz mentioned this pull request Sep 1, 2021

Include #251 in the release notes for 1.0.0 #330

Merged

bluetech added a commit that referenced this pull request Oct 1, 2021

Merge pull request #330 from cjolowicz/patch-1

cc9e86f

Include #251 in the release notes for 1.0.0

simonw added a commit to simonw/til that referenced this pull request Dec 20, 2021

specname requires Pluggy 1.0.0 or higher

20e40b0

Refs pytest-dev/pluggy#251 Refs simonw/datasette#1575

bluetech mentioned this pull request Aug 14, 2023

One hookspec to many hookimpl (implementations that don't match spec function names) #218

Closed

RonnyPfannschmidt mentioned this pull request Aug 14, 2023

multi hook registration does not unregister #431

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add specname option to hookimpl #251

add specname option to hookimpl #251

tlambert03 commented Jan 27, 2020 •

edited

Loading

goodboy commented Jan 27, 2020

asottile left a comment

RonnyPfannschmidt commented Jan 27, 2020

tlambert03 commented Jan 27, 2020

RonnyPfannschmidt commented Jan 27, 2020

tlambert03 commented Jan 27, 2020

nicoddemus left a comment

asottile left a comment

tlambert03 commented Feb 4, 2020 •

edited

Loading

goodboy commented Feb 4, 2020

tlambert03 commented Feb 4, 2020

goodboy left a comment

goodboy Feb 5, 2020

tlambert03 Feb 5, 2020

goodboy Feb 5, 2020

tlambert03 Feb 5, 2020

tlambert03 commented Feb 5, 2020

goodboy commented Feb 5, 2020

tlambert03 commented Feb 5, 2020

goodboy left a comment

tlambert03 commented Feb 5, 2020

tlambert03 commented Feb 5, 2020

simonw commented Dec 20, 2021

add specname option to hookimpl #251

add specname option to hookimpl #251

Conversation

tlambert03 commented Jan 27, 2020 • edited Loading

goodboy commented Jan 27, 2020

asottile left a comment

Choose a reason for hiding this comment

RonnyPfannschmidt commented Jan 27, 2020

tlambert03 commented Jan 27, 2020

RonnyPfannschmidt commented Jan 27, 2020

tlambert03 commented Jan 27, 2020

nicoddemus left a comment

Choose a reason for hiding this comment

asottile left a comment

Choose a reason for hiding this comment

tlambert03 commented Feb 4, 2020 • edited Loading

goodboy commented Feb 4, 2020

tlambert03 commented Feb 4, 2020

goodboy left a comment

Choose a reason for hiding this comment

goodboy Feb 5, 2020

Choose a reason for hiding this comment

tlambert03 Feb 5, 2020

Choose a reason for hiding this comment

goodboy Feb 5, 2020

Choose a reason for hiding this comment

tlambert03 Feb 5, 2020

Choose a reason for hiding this comment

tlambert03 commented Feb 5, 2020

goodboy commented Feb 5, 2020

tlambert03 commented Feb 5, 2020

goodboy left a comment

Choose a reason for hiding this comment

tlambert03 commented Feb 5, 2020

tlambert03 commented Feb 5, 2020

simonw commented Dec 20, 2021

tlambert03 commented Jan 27, 2020 •

edited

Loading

tlambert03 commented Feb 4, 2020 •

edited

Loading