Comments (20)
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 :)
from pluggy.
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.
from pluggy.
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
from pluggy.
@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 :)
from pluggy.
@tgoodlet nonetheless we would expose something that cannot be used in a sane context without deliberation, as such i#d rather not expose it
from pluggy.
@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.
from pluggy.
@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
from pluggy.
@RonnyPfannschmidt I really do believe that explaining how hookwrapper
s 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.
from pluggy.
@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 ;)
from pluggy.
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
from pluggy.
@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 Future
s but the user should never do so.
from pluggy.
@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
from pluggy.
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
from pluggy.
@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"?
from pluggy.
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
from pluggy.
@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()
?
from pluggy.
Backward compat is a pain,
removing the exposure to calloutcome would be fabulous but this needs a transition plan
from pluggy.
Also known AS this one will take at least 5 years
from 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()?
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.
from pluggy.
@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.
from pluggy.
Related Issues (20)
- Avoid erroring from tracing HOT 4
- Drop Python 3.7
- Export our typing HOT 2
- multi hook registration does not unregister HOT 6
- Can plugins be executed concurrently? HOT 3
- RFE: is it possible to start making github releases?🤔 HOT 3
- pytest won't run root conftest.py's pytest_configure in special circumstances HOT 2
- Rename misleading function name `PluggyManager.load_setuptools_entrypoints` HOT 3
- Issue with ordering of hooks HOT 2
- Question about default values in hook implementation HOT 1
- discussion: pluggy speedup by using 'codegen' HOT 7
- Is it possible to override an existing hook with a HookCaller class?
- pluggy loads disabled pytest plugins HOT 5
- Warn when old-style hookwrapper raises during teardown
- Add python-lsp-server to downstream tests
- Troubles to understand the new updates on Pluggy HOT 11
- Bug: Pluggy 1.4 searches all home dirs for __init__.py HOT 2
- Possible to tell which hook answered a call? HOT 1
- load_setuptools_entrypoints messing up with rpath HOT 2
- How to get in touch regarding a security concern HOT 6
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from pluggy.