disnake icon indicating copy to clipboard operation
disnake copied to clipboard

refactor: Improve webhook typing.

Open elenakrittik opened this issue 1 year ago • 8 comments

Summary

Fixes #626

This is a typing-breaking change.

Should Webhook.edit and Webhook.delete also be overloaded with a Never somewhere to forbid calling them (you can't delete or edit an interaction webhook, right?)?

Checklist

  • [x] If code changes were made, then they have been tested
    • [x] I have updated the documentation to reflect the changes
    • [x] I have formatted the code properly by running pdm lint
    • [x] I have type-checked the code by running pdm pyright
  • [x] This PR fixes an issue
  • [ ] This PR adds something new (e.g. new method or parameters)
  • [ ] This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • [ ] This PR is not a code change (e.g. documentation, README, ...)

elenakrittik avatar May 15 '24 18:05 elenakrittik

It's not quite clear whether this is meant to be a typing or runtime change; followup is annotated as returning an InteractionFollowupWebhook, but returns a "plain" Webhook in practice. Types that are only used in annotations can be confusing to some people, so it might be better to actually use this type at runtime too?

Making it actually return an InteractionFollowupWebhook will make it a runtime breaking change. I intentionally avoided that "by default" but if it's okay in this case i'll willingly do this!

elenakrittik avatar May 19 '24 12:05 elenakrittik

Making it actually return an InteractionFollowupWebhook will make it a runtime breaking change. I intentionally avoided that "by default" but if it's okay in this case i'll willingly do this!

ah, gotcha. :thinking: In that case, what about moving InteractionFollowupWebhook.send into a type-checking block as well? Similar to Bot.__init__, such that the runtime implementation is inherited from the parent class, while being able to adjust the (typing-only) method signature freely.

shiftinv avatar May 19 '24 13:05 shiftinv

Making it actually return an InteractionFollowupWebhook will make it a runtime breaking change. I intentionally avoided that "by default" but if it's okay in this case i'll willingly do this!

ah, gotcha. 🤔 In that case, what about moving InteractionFollowupWebhook.send into a type-checking block as well? Similar to Bot.__init__, such that the runtime implementation is inherited from the parent class, while being able to adjust the (typing-only) method signature freely.

That's a nice idea, thanks!

elenakrittik avatar May 19 '24 13:05 elenakrittik

Done in ec9a610259e3a4aa1c4eaa50ecdbcfe0ab81af72!

elenakrittik avatar May 19 '24 13:05 elenakrittik

having to subclass something to change a return type annotation for specific situations is rather awkward

This is why those situations should be working on distinct types. Not a subclass of one handle-all type, as it later shows the interface allows for more than is actually supported.

Enegg avatar May 19 '24 13:05 Enegg

having to subclass something to change a return type annotation for specific situations is rather awkward

This is why those situations should be working on distinct types. Not a subclass of one handle-all type, as it later shows the interface allows for more than is actually supported.

So you suggest something like making InteractionFollowupWebhook the base type and Webhook as sub-type?

elenakrittik avatar May 19 '24 13:05 elenakrittik

So you suggest something like making InteractionFollowupWebhook the base type and Webhook as sub-type?

I suggest making InteractionFollowupWebhook one type and Webhook another type. Only then consider creating some base/mixin class.

Enegg avatar May 19 '24 14:05 Enegg

This is why those situations should be working on distinct types. Not a subclass of one handle-all type, as it later shows the interface allows for more than is actually supported.

Yeah. A bunch of the class/instance methods on Webhook don't work or make sense in the context of application webhooks. Splitting it into separate types would be great, but is simultaneously the most breaking change

shiftinv avatar May 19 '24 15:05 shiftinv