-
Notifications
You must be signed in to change notification settings - Fork 121
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
Document the pluggy._Result
API
#49
Comments
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 |
@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 That's not even going into the detail that it only defines 2 methods - Let me know what you think :) |
@tgoodlet nonetheless we would expose something that cannot be used in a sane context without deliberation, as such i#d rather not expose it |
@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 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 |
@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 |
@RonnyPfannschmidt I really do believe that explaining how Again I'll make the comparison with It's not like a user would ever (maybe someone has?) instantiated their own |
@RonnyPfannschmidt you mention
I would argue there is no "lock in". We can always change the type of |
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 |
@RonnyPfannschmidt you wrote:
But the docs for
So it is the same concept. Yes, the |
@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 |
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 |
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? |
I agree with @RonnyPfannschmidt that we should not expose the This is probably easy to add to Sphinx, using the |
@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 This current API question will become relevant as we will need to deliver a |
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 |
@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 Also I'm not seeing this shoot my foot business in Given that you guys want to keep it private, do we even have a good reason not to just yield the value from |
Backward compat is a pain, |
Also known AS this one will take at least 5 years |
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. |
@RonnyPfannschmidt @nicoddemus good call on this you guys. I ended up needing to swap out |
pluggy._CallOutcome
as part of the public APIpluggy._Result
API
Currently
pluggy._CallOutcome
is implied to be part of the internal api by the leading_
.In practice it is used by
hookwrappers
.An example:
The
outcome
is accessed directly by the user.Therefore I propose we expose it correct in
plugg.__all__
and expose it in the api reference.The text was updated successfully, but these errors were encountered: