Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question about the "right" way for writing plugins #130

Closed
obestwalter opened this issue Apr 15, 2018 · 7 comments
Closed

Question about the "right" way for writing plugins #130

obestwalter opened this issue Apr 15, 2018 · 7 comments

Comments

@obestwalter
Copy link
Member

obestwalter commented Apr 15, 2018

I started looking into pluggy more and thought about how to use it right from the perspective of a plugin writer. I looked through the docs and came to the conclusion that the right way of doing stuff is to use the pluggy API to create a hookimpl object instead of importing it from the host project. I started changing the tox code and opened a whole slew of PRs to change to the "right" way. Long story short - I might have been a bit trigger happy and now I would like to clarify this and then either go ahead or undo the damage. My opinion at the moment is that using the pluggy API explicitly is the right way rather than importing the hookimpl from the host. In tox we already have the problem of several hookimpl objects that are imported by plugins. So I am talking about

import pluggy
hookimpl = pluggy.HookImplMarker("project")

vs

from project.where.ever.it.lives import hookimpl

@ryanhiebert argues that the plugin writer doesn't have to care and should not have to do this and instead get the hookimpl marker directly from the host. I have to admit that this makes a lot of sense, but atm I am still torn, so I would be interested what the official view of the core devs is. Reading the docs again with that perspective in mind it seems clear that hookimpl should be imported. Also taking this further means that the underlying plugin implementation could be replaced as long as the marker object, decorator args and passed in objects stay the same, so importing the object might be the better idea from that perspective also.

Once this is clarified one or the other way I would volunteer to enhance the docs to make it clearer how this should be done and why.

Also cc @asottile who already started using that approach in another project that is not in tox-dev.

affected PRs:

tox-dev/detox#12
https://github.com/tox-dev/tox-durations/pull/3
tox-dev/tox-pipenv#40
tox-dev/tox-travis#103

@asottile
Copy link
Member

Not that it's a big selling point here, but as a plugin writer I don't particularly care what plugin infra is being used by the host project. The second option both abstracts away "tox is using pluggy" and would enable tox to swap out plugin implementations (if need be) later on.

That said, either way seems fine to me. Just a minor flexibility/abstraction benefit from the latter.

@goodboy
Copy link
Contributor

goodboy commented Apr 16, 2018

This is actually a very good question that digs to an even more interesting topic of why there is the notion of a project name passed to HookImplMarker and PluginManager in the first place. Without noticing that difference there is no reason to choose one method over the other (calling the API from the client project with versus importing it from the host project).

It would probably be best if @hpk42 could comment as he is the original designer but I'll give my 2 cents after having maintained the code base a while.

With pluggy.HookImplMarker("project") and pluggy.PluginManager("project"), the project_name arg (here "project") is a namespacing/association mechanism that is used for creating combined and adjacent plugin spaces. That is, it's a way to associate marks to managers using a key defined by the user such that multiple plugin sets can be defined and use together in the same system; the PluginManager is not supposed to be a singleton.

Most often the way you see it's usefulness is in projects that use pluggy whose dependencies also use pluggy resulting in > 1 pluggy.PluginManager instances being used simultaneously. I don't think I've yet seen a project using pluggy that has more than one PluginManager internally although conceivably you could use it for some kind of internal plugin spec versioning (say you wanted to deliver some next-gen hook set alongside the current version that was opt-in for library consumers) or you could share implementation functions between multiple plugin sets / managers (again have yet to see this used in the wild) like some kind of polymorphic plugins spec.

To be honest I've always kind of thought that the project_name argument to the manager and marks was a bit strange because you are expecting the user to associate marks to managers manually. The same thing could have been accomplished by having each PluginManger generate marks for the user:

from pluggy import PluginManager

ngpm = PluginManager()
hook = ngpm.mark('nextgen')
pm = PluginManager()
legacy_hook = pm.mark('legacy')

such that in your non-host project,

from relies_on_pluggy import legacy_hook, hook

@legacy_hook
def myhook(arg):
    return do_legacy_stuff(arg)

@hook
def myhook(arg):
    return do_new_stuff(arg)

And (I think) this would mean the second approach (importing from the host project) would be that much more obvious and would expose the user to less pluggy internals as well as avoiding any potential name collisions (use id(PluginManager) as the value to each HookimplMarker). The only reason I can think for not doing this is because you must instantiate the manager at run-time instead of import time but I can't think of any reason why that would be true.

@RonnyPfannschmidt @hpk42 @nicoddemus I'd be interested to hear your opinions on this.

To answer your questions:

argues that the plugin writer doesn't have to care and should not have to do this and instead get the hookimpl marker directly from the host.

Either way the HookImplMaker instance referenced is a pluggy specific object; whether or not the pluggy API is called by the non-host project is really a matter of your own encapsulation preferences. The only down side is if the host project decides to change the project_name argument for some reason (lets say more managers/plugin-spaces are added for whatever reason) the consuming project's API call will have to change.

Because of this point that pluggy based projects may want to change their internal plugin namespacing I do think the latter option (importing the mark object directly) is probably the more maintainable approach.

The second option both abstracts away "tox is using pluggy" and would enable tox to swap out plugin implementations (if need be) later on.

Well not unless the new impl works just like pluggy or do you mean swap out pluggy implementations behind the scenes? I would say it's not explicitly requiring the consuming project to import anything from pluggy, sure.

@RonnyPfannschmidt
Copy link
Member

RonnyPfannschmidt commented Apr 16, 2018

a pluginmanager instance should never be required for declaring parts of a plugin,
py.test uses nested instances of plugin managers for selftesting, others may use similar setups

currently pluggy is severely limited wrt deprecating and evolving hooks,
unfortunately i cant find the time to spec out the variosu detail issues at the moment
(there is hook argument changes, hook call in conventions, hook deprecations alltogether)

replacing a plugin system tends to be a breaking change to any software (while its theoretically possible not to make it so, its immensely unlikely to pull it off)

i dont mind helping something like that, but i certainly dont want to take part in designing it as general mechanism

wrt the right way to write plugins - you declare the markers in the host project, and plugins import them from there

since there is a certain kind of duplication between the hookimpl/spec markers and pluginmanager i wonder if that burden can be elevated

but it cnt be elevated by moving it to pluginmanager, since pluginmanager is something whose instances are to be distinct from plugin spec/impl declarations

@obestwalter
Copy link
Member Author

obestwalter commented Apr 16, 2018

Hey thanks for the enlightenment.

From where I stand (not very deeply steeped in pluggy knowledge) it looks like you are considering a lot of things that are interesting from a "plugin provider" perspective (sorry if I don't have the terminology down correctly yet ...) e.g. tox or pytest, but are not important from the perspective of a plugin writer. A lot of it is flying miles over my head :)

So my short takeaway is: always import the HookImplMarker object(s) from the host project or hook provider or whatever you call them (unless you have a good reason to do something different and you know what you are doing). It is the job of the plugin/hookimpl provider to tell the plugin writer exactly how and where to get it from. Obvious und already somehow conventional location and name is <project>.hookimpl

amiright?

I'll undo the damage I did already and will do my best to clear up the confusion I might have caused :)

I will read through the docs more today and then open a PR with some docs specific for the person who comes to pluggy as someone who "just wants to write a little plugin" for e.g. tox and lay down the basics in a hopefully unambiguous way.

@obestwalter
Copy link
Member Author

@RonnyPfannschmidt s/makers/markers/

@obestwalter
Copy link
Member Author

@tgoodlet thanks a lot for your thoughts. I hope to understand them better when getting to know pluggy from the perspective of extending tox hooks.

@obestwalter
Copy link
Member Author

I guess this can be closed then. I opened a PR to help other newcomers here: #136

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants