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

How to best add and act on custom metadata for hookspec_opts and hookimpl_opts #133

Closed
justanr opened this issue Apr 16, 2018 · 5 comments
Closed
Labels

Comments

@justanr
Copy link

justanr commented Apr 16, 2018

I'm looking to figure out the best way to deprecate a hook we have provided to end users and might be implemented in some plugins already. In a more general sense, I want to be able to add my own metadata to spec and impl opts and have ways of acting on that inside pluggy itself.

For the record, I have considered registering a try-first implementation or hook wrapper that raises the deprecation warning. And that's the easy answer, but I'd like to be able to include this information in documentation of the spec itself (potentially through a sphinx plugin or something similar) so inserting it into the spec_opts seemed like the wiser thing to do. From there, I'd also like to be able to act on it in the hook caller.

However, I found this difficult mostly because of that static calls to _HookCaller that required copying large amounts of code into a subclass. Some of it could be cut down -- for example _HookCaller.__call__ doesn't need to be overridden only _HookCaller._hookexec needs to be intercepted and wrapped.

I understand that I'm poking at things that are probably not intended to be poked at, or at least weren't considered open to being poked at. But is there potential for opening them up to poking?

@goodboy
Copy link
Contributor

goodboy commented Apr 16, 2018

But is there potential for opening them up to poking

@jaraco I'm a massive fan of some good poking 😄; as long as we've got tests most good ideas are greatly welcome!

A built-in hook deprecation system is something I think we can definitely consider though I'd like to get feedback from @nicoddemus and @RonnyPfannschmidt on your suggestions.

My thoughts:

  • really like the idea from MetadataHookspecMarker; I think it's totally fine to pass through extra opts via **kwargs (I'd say PR this in asap as it's non-invasive)
  • the DeprecatedHookCaller is very interesting, particularly subclassing just to add precall() - this might be one of those motivating example to make pluggy self-hosting where we add pluggy hooks to pluggy itself?
  • regarding InstrumentedManager I totally think we should allow overloading the _HookCaller type and I think the addition of hook_caller_class (I would go with hook_caller_type) is completely warranted

Thanks for this feedback it's always nice to see some interest in extending the feature set 👍

@justanr
Copy link
Author

justanr commented Apr 16, 2018

I'm curious what a self hosting pluggy looks like. I was thinking more along the lines of potential cooperative multiple inheritance than that route though.

Depending on how others feel, the earliest I could put together good PRs with tests would be this coming weekend.

@goodboy
Copy link
Contributor

goodboy commented May 17, 2018

@justanr given that @RonnyPfannschmidt added the deprecation support you desired directly, do we need to look at this any further?

The remaining features I can see being proposed here are:

  • the ability to pass in additional mark data to HookspecMarker via **kwargs
  • changing the PluginManager to take in a hook_caller_type arg such that a user can implement a custom hook caller

Given that both of these requirements were a result of your need for the deprecation mark (which is now supported) would you still have reasons to move forward with these features?

@RonnyPfannschmidt @nicoddemus I personally think the latter is an interesting addition particularly if we ever do want to support async / coroutine hook implementations. For example, implementing an AsyncHookCaller with trio would be trivial.

@justanr
Copy link
Author

justanr commented May 17, 2018

I don't have a direct need for after the depreciation PR went in. There's still potential use here, noted by your async suggestion and I'm willing to put the work in for the PRs if others think this is a good idea.

@goodboy
Copy link
Contributor

goodboy commented May 22, 2018

@justanr I've made #151 and #152 to follow up on this discussion.
Since the underlying need has been addressed I'm closing this.
Thanks again for the input!

@goodboy goodboy closed this as completed May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants