DisnakeDev / disnake

An API wrapper for Discord written in Python.
https://docs.disnake.dev
MIT License
721 stars 137 forks source link

refactor: Improve webhook typing. #1193

Open elenakrittik opened 5 months ago

elenakrittik commented 5 months ago

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

elenakrittik commented 5 months ago

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!

shiftinv commented 5 months ago

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.

elenakrittik commented 5 months ago

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 commented 5 months ago

Done in ec9a610259e3a4aa1c4eaa50ecdbcfe0ab81af72!

Enegg commented 5 months ago

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.

elenakrittik commented 5 months ago

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?

Enegg commented 5 months ago

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.

shiftinv commented 5 months ago

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