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

Improve docs: introduce roles and add another example #136

Merged
merged 16 commits into from
May 17, 2018
Merged

Improve docs: introduce roles and add another example #136

merged 16 commits into from
May 17, 2018

Conversation

obestwalter
Copy link
Member

@obestwalter obestwalter commented Apr 16, 2018

My first go at explaining pluggy from the perspective of three different roles introducing all important concepts and practices in a reading time well under 5 minutes. Please note the pending fixme regarding dependency handling.

@RonnyPfannschmidt
Copy link
Member

IMHO pubsub is not a analogy of pluggy
while it is a hook system, callers control the receiving (path based force out in pytest for example) and the invocation
also modelling components like hook-wrappers, first-result and historic have a play

personally i believe a more fitting analogy is calling them invocation of a call and participation in a call
the dependency management could indeed take some help, its mainly a problem that python is fundamentally broken there

a lib like pluggy really could use support for having multiple versions installed in parallel - dito for things like attrs

@RonnyPfannschmidt
Copy link
Member

also thanks to things like first-result and hook-wrapper, there is more like 5-6 roles (and i am getting sadder and sadder as this unravels)

@obestwalter
Copy link
Member Author

IMHO pubsub is not a analogy of pluggy

It is used in the docs though (as one of two analogies):

pluggy accomplishes all this by implementing a request-response pattern_ using function subscriptions and can be thought of and used as a rudimentary busless publish-subscribe_ event system.

-- http://pluggy.readthedocs.io/en/latest/

For that very reason I used publisher and subscriber to describe the two fundamental roles. I am not particularly fond of those terms in the context of pluggy either. I fully expected that there has to be a discussion of terminology, but to keep things easy I built up on what you folks already established instead of trying to reinvent the wheel.

personally i believe a more fitting analogy is calling them invocation of a call and participation in a call

That is not an analogy. That is a rudimentary go at establishing some original terminology specific to pluggy. So you would suggest something like publisher -> invocator, subscriber -> participator? That sounds a bit esoteric to me but I have no better idea yet either. If we would go into that direction though, I would suggest to completely drop the uses of any analogies and their terminology and instead establish the pluggy concepts and terminology directly.

the dependency management could indeed take some help, its mainly a problem that python is fundamentally broken there

I reserve judgement if Python is fundamentally broken in that respect and it does not help us with the question at hand. So what does this mean regarding plugin writers? I haven't thought about that aspect at all yet, because I was really hoping that the designer(s) of pluggy have a clear idea how this should work :).

also thanks to things like first-result and hook-wrapper, there is more like 5-6 roles (and i am getting sadder and sadder as this unravels)

I did not intend to unravel anything and frankly I don't really understand what that means in the context of this PR.

I am trying to explain the basic work flow from different perspectives in the simplest possible way to give newcomers a quick grasp of things. I am also trying to introduce some clear terminology that can be used in all further documentation/discussion. I would be interested in opinions from that perspective, so that we can get this into the docs. IMHO the docs really need it.

If you also need to introduce more roles later to describe more advanced concepts: knock yourself out, but we have to start somewhere (and those roles don't need to be explained in the introduction for newcomers either, do they?).

I can only speak from my pretty fresh experience with pluggy: If I would have had an introduction like this text when I came to pluggy completely clueless it would have helped me to get started and saved me some trouble.

@goodboy
Copy link
Contributor

goodboy commented Apr 17, 2018

@obestwalter the description of request-response pattern and publish-subscribe was my doing in the initial docs draft. Agreed those descriptions aren't technically correct though I was trying to make a kind of like comparison:

thought of and used as a rudimentary busless publish-subscribe_ event system.

Regarding,

That is not an analogy. That is a rudimentary go at establishing some original terminology specific to pluggy. So you would suggest something like publisher -> invocator, subscriber -> participator? That sounds a bit esoteric to me but I have no better idea yet either. If we would go into that direction though, I would suggest to completely drop the uses of any analogies and their terminology and instead establish the pluggy concepts and terminology directly.

invocator or caller and participator or implementer works for me ; totally think we should go this way particularly now that it seems there's more users of the project outside of pytest and friends.

Please feel free to change/establish the terminology we'd like to use moving forward in this PR 👍

@obestwalter
Copy link
Member Author

obestwalter commented Apr 17, 2018

Hey @tgoodlet - sounds good to me! I actually liked your analogies although they are unusual in the context we are in :)

So the actual explanation and the roles are o.k.? Anything missing? Any formulation problems (I am not a native speaker and tend to use too long sentences, because german).

I will let this percolate for a while in the hope that maybe someone comes up with some better terminology and then run with whatever we have at that point.

Copy link
Contributor

@goodboy goodboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@obestwalter Thanks so much for improving this. I think establishing a more plugin related set of terms is much better!

docs/index.rst Outdated
(spring 2018) ``pluggy`` is not at 1.0 yet, so breaking changes should be
expected with any minor release).

They need to learn about ``pluggy`` and its capabilities and should at least
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This snippet seems out of place?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its structured in a way that the last paragraph always gives info about the needed pluggy knowledge. I will make this explicit then.

docs/index.rst Outdated
calls to their specified hooks in appropriate phases of program execution.
``pluggy`` will then do all the magic.

**FIXME not sure what is the right advice here. Do I need to vendor in pluggy as a publisher to avoid versioning conflicts? Should I just depend on pluggy without version and hope all is good?**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say latter is what we recommend; so far I don't think we've had any breaking changes particularly because pytest and friends are all dependants.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

o.k. that might be the sanest approach then :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had a look what tox does (never bothered to check) and tox actually pins the pluggy version to pluggy>=0.3.0,<1.0 ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pytest does 'pluggy>=0.5,<0.7' and devpi pluggy>=0.3.0,<1.0 😱

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this regard I think we can all agree on pinning >=0.x,<1.0; we definitely don't want to introduce changes that break pytest, devpi or tox even in pre-1.0.

And after all we are all the developers of the same projects, so we can all agree to postpone any breaking changes until 1.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wouldn't want to put that limit on per se - but we certainly can coordinate fallout

technically we should probably do a 1.0 already since pluggy is in productive use, and ever since https://0ver.org/ its painfully oblivious that that is just wrong ^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0ver.org ... 😆 reminds me of flask. We're using that in production since 2011 and it is at 0.12.2 atm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically we should probably do a 1.0 already since pluggy is in productive use,

Agree, let's make the next one 1.0 already then, or are there any significant features to be added before that?

docs/index.rst Outdated

There are three roles in which people might interact with ``pluggy``.

Publisher
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe actually Host as in plugin host like described here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I caught myself writing host at several points, so this is the right way to go I guess. I will read up a bit more about plugin terminology also ...

docs/index.rst Outdated
They need to learn about ``pluggy`` and its capabilities and should at least
loosely follow the development of the ``pluggy`` project.

Subscriber
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Provider or Implementer as in plugin provider?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implementer feels also right to me.

docs/index.rst Outdated
`Users` don't need to know that the mechanism making this possible is provided by ``pluggy``.

Origins: the ``pytest`` plugin system
*************************************
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to change the downstream language as well 👍

Copy link
Member Author

@obestwalter obestwalter Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - that was basically just working around the fact that I screwed up the structure by pluggin my intro on top. I will have a look at the whole thing then, when reworking the PR.

@RonnyPfannschmidt
Copy link
Member

true, i approached this far too grumpy, sorry for unloading that on you, you certainly don't deserve that

i believe the terminology of "invoke" and "receive" are important for the elevator pitch as
an follow-up should eventually bring in details like first-result and hook-wrappers

participation plays more of a role when those concepts are introduced

@obestwalter
Copy link
Member Author

hmm ... invoke and receive don't seem to belong together for me. Also looking at roles that would be invoker and receiver - receiver is much to passive for me.

I am definitely with @tgoodlet on the implementer term, also because that term is already established and hinted at in the naming scheme (hookimpl, HookImplMarker) which the implementer then uses. So implementer is the way to go on that side IMO.

Leaves us with the other side. That seems to be the harder part, because if we would stick with internal names, we'd end up with a specifier and that just sounds odd - and to be clear: I am talking about naming roles for people here. A person called specifier sounds more like a biologist discovering new types of bugs and naming them. Discovering bugs is something we have in common with biologists, but we wouldn't want to be called specifier, would we? :D

As @tgoodlet said: there is already established terminlogy, so lets have a look in Wikipedia:

The host application provides services which the plug-in can use, including a way for plug-ins to register themselves with the host application and a protocol for the exchange of data with plug-ins.

So the program is called host - that sounds right for the program, but I am not sure if calling the person working on that should be called hoster, but at least it's not terrible I guess, but maybe provider would be better?

Programmers typically implement plug-in functionality using shared libraries, which get dynamically loaded at run time, installed in a place prescribed by the host application.

Programmers implement plugin functionality and a pogrammer doing that, is in the role of the implementer. Ease. See above.

So at the moment I lean towards hoster and implementer. I updated the PR to mirror that, so you can read it again and see how that rings.

Copy link
Contributor

@goodboy goodboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@obestwalter looking better. I made another suggestion regarding the perspective for each role / use case.

Let me know what you think. I like that we're converging on a good explanation here!

docs/index.rst Outdated
Introduction
------------

To explain how pluggy can be put to use we need to introduce three roles and explain their interaction with pluggy:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To explain how ``pluggy`` is best utilized we need to introduce three use cases:

A suggestion: reading this over again I think it might be more clear to talk about the code/programs themselves instead of people who write or use the code. This way we talk about what host versus implementer (maybe just plugin?) versus user (or client) code should look like and how it fits into the pluggy use-case model.

Also, too many references to pluggy in a few places when we already know what's being referenced.

Copy link
Member

@nicoddemus nicoddemus Apr 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestions, I agree. 👍

Also 👍 to use plugin instead of implementer, I think it will make the text clearer (people who come looking for pluggy are already looking how to implement/integrate plugins IMHO).

docs/index.rst Outdated

To explain how pluggy can be put to use we need to introduce three roles and explain their interaction with pluggy:

* The `hoster` - the person who wants to add extensibility to their program
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The host - the program or project to be extended and augmented with plugins

docs/index.rst Outdated
To explain how pluggy can be put to use we need to introduce three roles and explain their interaction with pluggy:

* The `hoster` - the person who wants to add extensibility to their program
* The `implementer` - the person who wants to extend the `host` functionality by writing a plugin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementer - the plugin itself which extends or modifies the hosts functionality and/or behaviour

docs/index.rst Outdated

* The `hoster` - the person who wants to add extensibility to their program
* The `implementer` - the person who wants to extend the `host` functionality by writing a plugin
* the `user` - the person who installed the `host` and now wants to extend its functionality by using a plugin
Copy link
Contributor

@goodboy goodboy Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the user - the program who depends on the host project and now wants to extend its functionality by installing and using a plugin

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the only one that feels a bit wonky to me, but I already got it covered: I will apply the "Why not both?"-paradigm. Roles do not necessary have to be all programs or all humans. So we have actual 3 program roles and 1 human role (host, plugin and pluggy connecting both) + the poor little human user having to get this all to work in their venv.

docs/index.rst Outdated
Hoster
++++++

The `hoster` wants to enable an arbitrary number of `implementers` to extend
Copy link
Contributor

@goodboy goodboy Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with my suggestion the language will have to change a bit, eg:
The host enables an arbitrary number of plugins to extend its behaviour

@RonnyPfannschmidt
Copy link
Member

true, we have implementation, specification, and invokation

@obestwalter
Copy link
Member Author

I think it's a good idea to set the roles like you suggested @tgoodlet - if @RonnyPfannschmidt is o.k. with the terms (host, implementer and user) for the fundamental roles, I will go ahead and adjust the docs that way.

@RonnyPfannschmidt
Copy link
Member

so far this is good work, i will try to review in detail this weekend, thanks!

@obestwalter
Copy link
Member Author

Hi @RonnyPfannschmidt thanks for offering to review, but please hold your horses. I am in the middle of rewriting this and I am not sure, if I get around to finishing it this weekend. I think I have a good idea now how to get the basic concepts over succinctly and easy to digest, but I need a bit longer.

@RonnyPfannschmidt
Copy link
Member

@obestwalter thanks for the note, please ping me later on when you feel confident in your reiteration

also 👍

Rework the beginning of the docs answering three fundamental questions.

Instead of trying to cram everything into a highly compressed role definition, do the following:

Use 4 roles to introduce the basic concept from a bird's eye perspective - 3 of them are programs and one of them is a human (because why not?).

Instead of having only one example have two: the first example becomes
the "toy example" (because that's what it is). Add another example that
is showing two minimal projects (host and plugin), demonstrating the
concepts that would overwhelm the first toy example but are important
to understand conventions and get a complete picture.

Some additional changes in the docs:

* Flesh out the motivation for a system like pluggy a bit more.
* Turn "The pytest plugin system" from a heading into a tag-line.
* fix a pre existing title level problem
* add orphan tag to api reference to be able to build with warnings as errors
* turn doc build warnings into errors
* Made title casing more consistent
@obestwalter obestwalter changed the title Add an "elevator pitch" introducing pluggy via roles Improve docs: introduce roles and add another example Apr 22, 2018
@obestwalter
Copy link
Member Author

obestwalter commented Apr 22, 2018

Hey @RonnyPfannschmidt and @tgoodlet - whaddaya think? It got a bit out of hand and I fixed a few more things along the way. Seems like small focussed PRs are not my strong suit ...

Also cc @fschulze: is that the right way to link to the devpi dev docs or is there a better way? I had to break out of the frame to get something that worked: https://github.com/pytest-dev/pluggy/pull/136/files#diff-caf2a6b8f4947d018f68893c695b5202R165

@fschulze
Copy link
Contributor

@obestwalter it's better to use "+d" instead of "+doc":
https://devpi.net/docs/devpi/devpi/stable/+d/devguide/index.html

There will be some documentation related fixes in the next devpi-web release regarding linking and fragments etc.

@obestwalter
Copy link
Member Author

Thanks @fschulze -fixed.

Copy link
Contributor

@goodboy goodboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a few suggestions for slight changes but otherwise I'd say this is shaping up to be quite eggsellent work.


from eggsample import hookimpl, hookspecs, lib, pm

condiments_tray = {"pickled walnuts": 13, "steak sauce": 4, "mushy peas": 2}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man, gotta try me some pickled walnuts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a Wikipedia approved british condiment, so it must be good!

pm.add_hookspecs(hookspecs)
pm.load_setuptools_entrypoints("eggsample")
pm.register(lib)
fm2000 = FoodMaster2000()
Copy link
Contributor

@goodboy goodboy Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I love the name FoodMaster2000 definitely seems like one of those cases where you ask why is this a class and not just functions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true. Was more complex before and I whittled it down to the absolutely necessary and now it should be just functions. Will fix that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed in pluginmanager and did it similar to how real world classes handle it in pytest or tox. Gives the class a reason to exist now (but I couldn't help but chaging the name using your pun :)).

(e.g. Jinja2) or
`monkey patching <https://en.wikipedia.org/wiki/Monkey_patch>`_ (e.g. gevent
or for `writing tests <https://docs.pytest.org/en/latest/monkeypatch.html>`_).
These strategies become problematic though when several parties want to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo on this description; well done!

docs/index.rst Outdated
- the hook :ref:`implementations <impls>` provided by registered
``plugins`` (a.k.a ``hookimpl`` - see `callbacks`_)
- the hook :ref:`caller <calling>` - a call loop triggered at appropriate
program positions in the ``host`` invoking the implementations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invoking the implementations **and** collecting the results?

docs/index.rst Outdated
be prepared and served with a tray containing condiments. As everybody knows:
the more cooks are involved the better the food, so let us make the process
pluggable and write a plugin that improves the meal with some spam and removes
the steak sauce from the condiments tray (nobody likes that anyway).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lolz definitely not on eggs.

docs/index.rst Outdated
subscriptions and can be thought of and used as a rudimentary busless `publish-subscribe`_
event system.
**naming markers**: ``HookSpecMarker`` and ``HookImplMarker`` must be
the initialized with the name of the ``host`` project (the ``name``
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop the

docs/index.rst Outdated
inside Plugin_2.myhook()
inside Plugin_1.myhook()
[-1, 3]
.. literalinclude:: examples/eggsample/eggsample/lib.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally like including line numbers with :linenos:.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah - yes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it ... and removed it again. Looks absoulutely horrific due to sphinx-doc/sphinx#2427 and does not really add too much utility IMO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh, not that big of a deal. Yeah just avoid the headache.

@obestwalter
Copy link
Member Author

I am getting quite eggsited now!

hookimpl = pluggy.HookimplMarker("eggsample")
"""Marker to be imported and used in plugins"""

pm = pluggy.PluginManager("eggsample")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we take a note here that a pluginmanager instance is typically not a global ?

Copy link
Member Author

@obestwalter obestwalter Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was not happy with that bit either - I will see, if I can make it a bit more real world like regarding this, so it is part of some mini config. If this adds too much cruft, I will at least add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it in the main and pass it into the class similar to how its done in real world projects now.

* fix typos
* use pluginmanager in a fashion closer to real world usage
* normalize whitespace after titles in docs
@goodboy
Copy link
Contributor

goodboy commented Apr 30, 2018

@obestwalter where are we with this - all done?

@nicoddemus @RonnyPfannschmidt are you two happy?

@obestwalter
Copy link
Member Author

I'd say done ... except for maybe reading the complete docs as a whole making sure the terminology is consistent. I did not do that in this PR, as I think this one is huge enough. I would definitely like to have a go at this in another PR if you think it is a good idea.

@nicoddemus
Copy link
Member

I think this is good to go, as much as my fever-induced review allows 😅 (having a flu that comes and goes for the past week sadly)

@obestwalter
Copy link
Member Author

Hi @nicoddemus, thanks for having a look and get well soon.

Copy link
Contributor

@goodboy goodboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple more nitpicks @obestwalter. Feel free to debate me on them.

It's so close!

@eggsample.hookimpl
def eggsample_prep_condiments(condiments):
try:
del condiments["steak sauce"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

condiments.pop('steak sauce', None) is maybe a little more succinct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You think so? I'd say you use del if you want to get rid of something and list.pop if you want to do something with the value. Here the steak sauce is just removed from the tray to vanish into thin air, so I'd use del here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's dict.pop but sure. I don't mind either way too much.


condiments_tray = {"pickled walnuts": 13, "steak sauce": 4, "mushy peas": 2}

class EggselentCook:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a play on excellent then 2 ls yeah?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah, thanks :)

self.ingredients = []

def add_ingredients(self):
self.hook.eggsample_add_ingredients(ingredients=self.ingredients)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm wondering, might it be better to showcase how each hookimpl returns values from the hook call?
So instead of having eggsample_add_ingredients() mutate the ingredients list instead have each return their respective entries and have Eggsellent.add_ingredients() do the extending? I think it's important to indicate that each hook call returns values from each hook implementation in sequence.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, will adjust this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original intention here was to make clear that due to Pythons behaviour of passing by assignment it is possible to mutate the object that pluggy passes to the client, which is also used regularly e.g. when adding command line parameters to the parser, but I agree that in the context of pluggy it is more important to showcase the return behaviour.

docs/index.rst Outdated

The ``pluggy`` approach puts the burden on the designer of the
``host program`` to think carefully about which objects are explicitly needed
by an implementation and gives designers of the ``plugin`` a clear framework
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight edit suggestion to minimize wordiness:

The pluggy approach puts the burden on the designer of the host program to think carefully about which objects are explicitly needed by a plugin implementation and gives plugin creators a clear framework for how to extend the host by providing a well defined set of hookable functions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tweaked it a bit more, hope it's ok like this.

docs/index.rst Outdated
* ``host`` or ``host program``: the program offering extensibility
by specifying ``hook functions`` and invoking their implementation(s) as
part of program execution
* ``plugin``: the program implementing a subset of the specified hooks and
Copy link
Contributor

@goodboy goodboy May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a subset of should maybe be inside () since technically a plugin could implement all hooks.
Though I guess a set is a subset of itself?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thoughts myself and came to the latter conclusion, but if you had to do the same mental gymnastics, I guess it is better to add the parens :)

@obestwalter
Copy link
Member Author

Hi @tgoodlet thanks for the comments - will fix that up over the weekend and hopefully it will be part of 1.0 then :)

@obestwalter
Copy link
Member Author

@tgoodlet - thanks for the suggestions, from my side this is good to go now.

@obestwalter
Copy link
Member Author

I can be awfully repetitive sometimes - but I guess you can never say thanks too often :)

def add_ingredients(self):
more_ingredients = self.hook.eggsample_add_ingredients(
ingredients=self.ingredients)
# each hook implementation returned a list of ingredients
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@obestwalter hmm so more_ingredients will now be a list of lists - I'm not sure self.ingredients will now be the same as before so you're examples in the docs might be wrong.

The easy way to fix this might be to do:

for ingredients in self.hook.eggsample_add_ingredients(ingredients=self.ingredients):
    self.ingredients.extend(ingredients)


def serve_the_food(self):
self.hook.eggsample_prep_condiments(condiments=condiments_tray)
print(f"Your food: {', '.join(self.ingredients)}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will now join a bunch of list items with a , If I'm not mistaken since each hookimpl returns a list and you're extending self.ingredients with the list of lists.

docs/index.rst Outdated

Add ['egg', 'egg', 'salt', 'pepper']
add ['lovely spam', 'wonderous spam', 'splendiferous spam']
Your food: egg, lovely spam, egg, pepper, salt, wonderous spam, splendiferous spam
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah just double check this with a test to make sure either I'm wrong or this needs to be fixed.

* fix bug introduced when "fixing" the example
* reorder code: give the big picture first (main())
* split out function to get plugin manager
* use the two functions to demonstrate returning a result vs modifying
  an argument
* avoid too lont sentences.
* use pip --editable instead of -e for clarity
@obestwalter
Copy link
Member Author

obestwalter commented May 15, 2018

o.k. unless I managed to introduce another bug while improving it, I would say this is good to go.

Copy link
Contributor

@goodboy goodboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lookin good let's get it live and see how it views!

@goodboy
Copy link
Contributor

goodboy commented May 16, 2018

@RonnyPfannschmidt do you need one more look over or can we merge this?

@nicoddemus
Copy link
Member

Definitely, awesome work @obestwalter and @tgoodlet on reviewing! 👍

@goodboy goodboy merged commit 119836b into pytest-dev:master May 17, 2018
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

Successfully merging this pull request may close these issues.

None yet

5 participants