Skip to content

feat(Webhook)!: New classes PartialWebhook & PartialSyncWebhook #878

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/622.breaking.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Migrate ``fetch`` methods of :class:`Webhook` and :class:`SyncWebhook` to :class:`PartialWebhook` and :class:`PartialSyncWebhook`.
Copy link
Contributor

@elenakrittik elenakrittik May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Migrate ``fetch`` methods of :class:`Webhook` and :class:`SyncWebhook` to :class:`PartialWebhook` and :class:`PartialSyncWebhook`.
Move ``fetch`` methods of :class:`Webhook` and :class:`SyncWebhook` to :class:`PartialWebhook` and :class:`PartialSyncWebhook`.

1 change: 1 addition & 0 deletions changelog/622.docs.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Documents ``TypeError`` raised on invalid session passed for :meth:`PartialSyncWebhook.from_url`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Documents ``TypeError`` raised on invalid session passed for :meth:`PartialSyncWebhook.from_url`.
Document ``TypeError`` raised on invalid session passed for :meth:`PartialSyncWebhook.from_url`.

2 changes: 2 additions & 0 deletions changelog/622.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Adds :class:`PartialWebhook` and :class:`PartialSyncWebhook` to represent partial webhook objects, in addition the :meth:`from_url`
and :meth:`partial` methods will now returns a partial webhook object that could be either a :class:`PartialWebhook` or :class:`PartialSyncWebhook`.
Comment on lines +1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Adds :class:`PartialWebhook` and :class:`PartialSyncWebhook` to represent partial webhook objects, in addition the :meth:`from_url`
and :meth:`partial` methods will now returns a partial webhook object that could be either a :class:`PartialWebhook` or :class:`PartialSyncWebhook`.
Add :class:`PartialWebhook` and :class:`PartialSyncWebhook` to represent partial webhook objects, in addition the :meth:`from_url`
and :meth:`partial` methods will now return a partial webhook object that could be either a :class:`PartialWebhook` or :class:`PartialSyncWebhook`.

In addition, i think this should be split into two separate changelog entries.

174 changes: 105 additions & 69 deletions disnake/webhook/async_.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"WebhookMessage",
"PartialWebhookChannel",
"PartialWebhookGuild",
"PartialWebhook",
)

_log = logging.getLogger(__name__)
Expand Down Expand Up @@ -960,6 +961,14 @@ def is_authenticated(self) -> bool:
"""
return self.auth_token is not None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much github for not allowing me to comment on any line. Actually, no, f*ck you, the feature request for this is dangling ever since 2021 and you did nothing about it

The Webhook.is_partial method just a bit above should be removed.


def __repr__(self) -> str:
return f"<Webhook id={self.id!r}>"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use something like self.__class__.__name__ in place of "Webhook" so that it evaluates to "<Webhook ...>" on Webhook objects and "<PartialWebhook ...>" on PartialWebhook objects. Alternatively, write separate __repr__ impls for Webhook and PartialWebhook.


@property
def url(self) -> str:
""":class:`str` : Returns the webhook's url."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
""":class:`str` : Returns the webhook's url."""
""":class:`str` : Get webhook's url."""

return f"https://discord.com/api/webhooks/{self.id}/{self.token}"

@property
def guild(self) -> Optional[Guild]:
"""Optional[:class:`Guild`]: The guild this webhook belongs to.
Expand Down Expand Up @@ -1095,19 +1104,18 @@ def __init__(
super().__init__(data, token, state)
self.session = session

def __repr__(self) -> str:
return f"<Webhook id={self.id!r}>"

@property
def url(self) -> str:
""":class:`str` : Returns the webhook's url."""
return f"https://discord.com/api/webhooks/{self.id}/{self.token}"

@classmethod
def partial(
cls, id: int, token: str, *, session: aiohttp.ClientSession, bot_token: Optional[str] = None
) -> Webhook:
"""Creates a partial :class:`Webhook`.
self,
Comment on lines -1106 to +1108
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self appears to be unused, so i believe this should be at least made a @staticmethod. More generally, i think this should also be moved to PartialWebhook (in that case it should remain a @classmethod)

id: int,
token: str,
*,
session: aiohttp.ClientSession,
bot_token: Optional[str] = None,
) -> PartialWebhook:
"""Creates a :class:`PartialWebhook`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""Creates a :class:`PartialWebhook`.
"""Create a :class:`PartialWebhook`.


.. versionchanged:: 2.8
Returns a :class:`PartialWebhook` instead of a `Webhook`.

Parameters
----------
Expand All @@ -1130,7 +1138,7 @@ def partial(

Returns
-------
:class:`Webhook`
:class:`PartialWebhook`
A partial :class:`Webhook`.
A partial webhook is just a webhook object with an ID and a token.
"""
Expand All @@ -1140,17 +1148,19 @@ def partial(
"token": token,
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do an isinstance(session, aiohttp.Session) check here to be consistent with SynchWebhook?

return cls(data, session, token=bot_token)
return PartialWebhook(data, session, token=bot_token)

@classmethod
def from_url(
cls, url: str, *, session: aiohttp.ClientSession, bot_token: Optional[str] = None
) -> Webhook:
"""Creates a partial :class:`Webhook` from a webhook URL.
self, url: str, *, session: aiohttp.ClientSession, bot_token: Optional[str] = None
) -> PartialWebhook:
Comment on lines -1145 to +1155
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as with Webhook.partial, this should at least be made a @staticmethod and ideally moved to PartialWebhook itself.

"""Creates a :class:`PartialWebhook` from a webhook URL.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""Creates a :class:`PartialWebhook` from a webhook URL.
"""Create a :class:`PartialWebhook` from a webhook URL.
``|


.. versionchanged:: 2.6
Raises :exc:`ValueError` instead of ``InvalidArgument``.

.. versionchanged:: 2.8
Returns a :class:`PartialWebhook` instead of a :class:`Webhook`.

Parameters
----------
url: :class:`str`
Expand All @@ -1175,7 +1185,7 @@ def from_url(

Returns
-------
:class:`Webhook`
:class:`PartialWebhook`
A partial :class:`Webhook`.
A partial webhook is just a webhook object with an ID and a token.
"""
Expand All @@ -1188,7 +1198,7 @@ def from_url(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same again: should we do an isinstance check on session somewhere here for consistency with SyncWebhook?

data: Dict[str, Any] = m.groupdict()
data["type"] = 1
return cls(data, session, token=bot_token) # type: ignore
return PartialWebhook(data, session, token=bot_token) # type: ignore

@classmethod
def _as_follower(cls, data, *, channel, user) -> Webhook:
Expand Down Expand Up @@ -1216,55 +1226,6 @@ def from_state(cls, data, state) -> Webhook:
session = state.http._HTTPClient__session
return cls(data, session=session, state=state, token=state.http.token)

async def fetch(self, *, prefer_auth: bool = True) -> Webhook:
"""|coro|

Fetches the current webhook.

This could be used to get a full webhook from a partial webhook.

.. versionadded:: 2.0

.. note::

When fetching with an unauthenticated webhook, i.e.
:meth:`is_authenticated` returns ``False``, then the
returned webhook does not contain any user information.

.. versionchanged:: 2.6
Raises :exc:`WebhookTokenMissing` instead of ``InvalidArgument``.

Parameters
----------
prefer_auth: :class:`bool`
Whether to use the bot token over the webhook token,
if available. Defaults to ``True``.

Raises
------
HTTPException
Could not fetch the webhook
NotFound
Could not find the webhook by this ID
WebhookTokenMissing
This webhook does not have a token associated with it.

Returns
-------
:class:`Webhook`
The fetched webhook.
"""
adapter = async_context.get()

if prefer_auth and self.auth_token:
data = await adapter.fetch_webhook(self.id, self.auth_token, session=self.session)
elif self.token:
data = await adapter.fetch_webhook_with_token(self.id, self.token, session=self.session)
else:
raise WebhookTokenMissing("This webhook does not have a token associated with it")

return Webhook(data, self.session, token=self.auth_token, state=self._state)

async def delete(self, *, reason: Optional[str] = None, prefer_auth: bool = True) -> None:
"""|coro|

Expand Down Expand Up @@ -1936,3 +1897,78 @@ async def delete_message(self, message_id: int, /) -> None:
message_id,
session=self.session,
)


class PartialWebhook(Webhook):
"""Represents an asynchronous Discord partial webhook to aid with working webhooks when only an ID and a token are present.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how correct this suggestion is, but the current wording reads half-odd, half-extraneous to me.

Suggested change
"""Represents an asynchronous Discord partial webhook to aid with working webhooks when only an ID and a token are present.
"""Represents a partial :class:`Webhook`.


PartialWebhooks are Webhook objects with an ID and a token.

The only way to construct this class is through :meth:`Webhook.from_url` and :meth:`Webhook.partial`.

Note that this class is trimmed down and has no rich attributes.
Comment on lines +1905 to +1909
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PartialWebhooks are Webhook objects with an ID and a token.
The only way to construct this class is through :meth:`Webhook.from_url` and :meth:`Webhook.partial`.
Note that this class is trimmed down and has no rich attributes.
The only way to construct this class is through :meth:`Webhook.from_url` and :meth:`Webhook.partial`.
Note that this class is trimmed down and has no rich attributes. :meth:`PartialWebhook.fetch` it to access the full object.


Attributes
----------
id: :class:`int`
The webhook's ID
token: Optional[:class:`str`]
The authentication token of the webhook. If this is ``None``
then the webhook cannot be used to make requests.
"""

def __init__(
self,
data: WebhookPayload,
session: aiohttp.ClientSession,
token: Optional[str] = None,
) -> None:
super().__init__(data, session=session, token=token)
Comment on lines +1920 to +1926
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This override doesn't look necessary?


async def fetch(self, *, prefer_auth: bool = True) -> Webhook:
"""|coro|

Fetches the current PartialWebhook to get a full Webhook.

This could be used to get a full webhook from a partial webhook.
Comment on lines +1931 to +1933
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Fetches the current PartialWebhook to get a full Webhook.
This could be used to get a full webhook from a partial webhook.
Fetch the full :class:`Webhook` from this partial webhook.


.. versionadded:: 2.8

.. note::

When fetching with an unauthenticated webhook, i.e.
:meth:`Webhook.is_authenticated` returns ``False``, then the
returned webhook does not contain any user information.
Comment on lines +1939 to +1941
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me think that Webhook.is_authenticated should be moved to BaseWebhook instead.


Raises :exc:`WebhookTokenMissing` instead of ``InvalidArgument``.

Parameters
----------
prefer_auth: :class:`bool`
Whether to use the bot token over the webhook token,
if available. Defaults to ``True``.

Raises
------
HTTPException
Could not fetch the webhook
NotFound
Could not find the webhook by this ID
WebhookTokenMissing
This webhook does not have a token associated with it.

Returns
-------
:class:`Webhook`
The fetched webhook.
"""
adapter = async_context.get()

if prefer_auth and self.auth_token:
data = await adapter.fetch_webhook(self.id, self.auth_token, session=self.session)
elif self.token:
data = await adapter.fetch_webhook_with_token(self.id, self.token, session=self.session)
else:
raise WebhookTokenMissing("This webhook does not have a token associated with it")

return Webhook(data, self.session, token=self.auth_token, state=self._state)
Loading