Document the `pluggy._Result` API

[pytest-dev]

Document the `pluggy._Result` API

[goodboy]

Ok fair enough; I guess I thought docs and the standard lib's multiple examples of this was sufficient but maybe you're right.

I guess maybe we should see what others think?
If the consensus is against me I'll of course concede :)

[nicoddemus]

I agree with @RonnyPfannschmidt that we should not expose the _CallOutcome class, but we should really document the public methods properly, since this is part of the public API and is used by hookwrappers.

This is probably easy to add to Sphinx, using the automethod directive IIRC.

[RonnyPfannschmidt]

imho we should not expose it, its an internal interface, only exposed as value to the user unable to change the behavior or make sensible use of own instances

all making it public would serve is handing people footguns they arent supposed to use

[goodboy]

@RonnyPfannschmidt are you sure about that?

Don't you think that preventing "shooting one's self in the foot" could be avoided by a proper private interface (i.e. set of methods) on CallOutcome?
For example in the exposure of this object we make CallOutcome.get_result() public but force_results() private (so I guess _force_result())?

That's not even going into the detail that it only defines 2 methods - get_result() and force_result() and that the latter is probably fine as a public method considering its name.
The standard library's concurrent.Future defines something similar and includes a analogous set_result() which is exposed publicly.

Let me know what you think :)

[RonnyPfannschmidt]

@tgoodlet nonetheless we would expose something that cannot be used in a sane context without deliberation, as such i#d rather not expose it

[goodboy]

@RonnyPfannschmidt I agree deliberation is good but what do you consider a "sane context" to be if the above example is not such a case?

I'm interested to know what you're thinking should be the requirement(s) for components in pluggy to be considered part of the public API?

I do agree it's not good to expose routines that allow the user to hurt themselves but I would argue that in this case consumers of CallOutcome may in fact want to use force_result for legitimate reasons. As an example pytest does this and I would consider pytest client code which consumes pluggy's public API.

[RonnyPfannschmidt]

@tgoodlet a public method is fine, but why should we expose a class that adds no value to the user while locking us in to having it when all we really do is expose a few public methods

i would go as far as shifting the api so a call-outcome is a opaque object and the methods to interact with it are importable functions instead

[goodboy]

@RonnyPfannschmidt I really do believe that explaining how hookwrappers work is much easier when you can point in the docs to a CallOutcome definition alongside usage of processing such an instance.

Again I'll make the comparison with concurrent.Future.

It's not like a user would ever (maybe someone has?) instantiated their own Future intance for use but having it documented and exposing it as part of the stdlib's public API definitely helps with understanding how to use it in practise.

[goodboy]

@RonnyPfannschmidt you mention

but why should we expose a class that adds no value to the user while locking us in to having it when all we really do is expose a few public methods

I would argue there is no "lock in". We can always change the type of CallOutcome without changing its public interface; that's the beauty of duck typing ;)

[RonnyPfannschmidt]

implementers os a futures Executors have to instantiate them, at pluggy, only the internals ever instantiate the outcomes

it doesnt make sense to expose the type for that reason - im not aware of anything where others require it

as soon as we expose a type changing it will break some peoples code and will always require a major release

[goodboy]

@RonnyPfannschmidt you wrote:

implementers os a futures Executors have to instantiate them, at pluggy, only the internals ever instantiate the outcomes

But the docs for concurrent.Future clearly state (and I know this from practise) that,

Encapsulates the asynchronous execution of a callable. Future instances are created by Executor.submit() and should not be created directly except for testing.

So it is the same concept. Yes, the Executor.submit() creates Futures but the user should never do so.

[RonnyPfannschmidt]

@tgoodlet that bacislly holds that there is 2 cases where Future objects need to be created outside of the codebase - impelemntign Executors, and testing

those do not hold for pluggy outcomes

[RonnyPfannschmidt]

unless there is a compelling case that makes it necessary to expose the type i am strictly against providing it and "the docs" is not a case that warrants it

[goodboy]

@nicoddemus @RonnyPfannschmidt cool I'll take a look at updating the docs the way you propose then.

Another question for you both is what happens if we want to eventually add built-in asynchronous hook calling to pluggy? For example leveraging asyncio.

This current API question will become relevant as we will need to deliver a Future like object as part of the hook calling API which imho is basically the direct analogue of _CallOutcome (but obviously in a synchronous context). I would even go further to say should we eventually move _CallOutcome further towards the notion of a Future to prevent duplication and unify the concept of a "future result"?

[RonnyPfannschmidt]

hookwrappers do not share the same needs as an actual future - i would consider trying to turn them into a future a premature de-duplication

from my pov outcomes for hookwrappers have a much different life-cycle pattern than futures and we should keep it in mind and separate

[goodboy]

@RonnyPfannschmidt yeah thinking more about this I agree with you; they are definitely distinct concepts that I'm mixing incorrectly.

I'm still struggling with this notion of a private instance that has "public methods" which are the only part we document. I don't think I've seen that pattern anywhere else.

I think my original quiff is that we're letting client code assign/reference/call an internal instance but you guys are saying we don't want to expose what it is only how to use it? To me how to use it is defined by the class definition just like any other object in Python.

Also I'm not seeing this shoot my foot business in _CallOutcome. Both public methods are used by client code in practice (eg. pytest) in a very safe and predictable way which can be well documented and explained to any user of pluggy.

Given that you guys want to keep it private, do we even have a good reason not to just yield the value from get_result() directly to the generator other then client code which chooses to use force_result()?

[RonnyPfannschmidt]

Backward compat is a pain,
removing the exposure to calloutcome would be fabulous but this needs a transition plan

[RonnyPfannschmidt]

Also known AS this one will take at least 5 years

[nicoddemus]

Given that you guys want to keep it private, do we even have a good reason not to just yield the value from get_result() directly to the generator other then client code which chooses to use force_result()?

Yep that's the main reason, but yielding an object also opens the path for future additions. If we just yielded the result value, we would never be able to expand that API.

[goodboy]

@RonnyPfannschmidt @nicoddemus good call on this you guys.

I ended up needing to swap out _CallOutcome for #58 and instead am using a new Result type so documenting the original would have been pointless.