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

Conversation

Snipy7374
Copy link
Collaborator

@Snipy7374 Snipy7374 commented Nov 26, 2022

Summary

Extras:

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running task lint
    • I have type-checked the code by running task pyright
  • 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, ...)

@shiftinv shiftinv added t: enhancement New feature breaking change Includes breaking changes to code/packaging s: needs review Issue/PR is awaiting reviews t: refactor/typing/lint Refactors, typing changes and/or linting changes labels Nov 27, 2022
@shiftinv shiftinv added this to the disnake v2.8 milestone Nov 27, 2022
@shiftinv shiftinv modified the milestones: disnake v2.8, disnake v2.9 Feb 5, 2023
@onerandomusername
Copy link
Member

@Snipy7374 would you please resolve conflicts?

@Snipy7374
Copy link
Collaborator Author

@Snipy7374 would you please resolve conflicts?

Oke

@Snipy7374 Snipy7374 changed the title feat(Webhook): New classes PartialWebhook & PartialSyncWebhook feat(Webhook)!: New classes PartialWebhook & PartialSyncWebhook Apr 13, 2023
@shiftinv shiftinv removed this from the disnake v2.9 milestone Jun 16, 2023
Copy link
Contributor

@elenakrittik elenakrittik left a comment

Choose a reason for hiding this comment

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

A few documentation nitpicks (mostly).

Also, i think it would make a lot more sense if [Sync]Webhook inherited from Partial[Sync]Webhook than the other way around. If maintainers/users are fine with a little more breakage, i'd do it.

@@ -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`.

@@ -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`.

Comment on lines +1 to +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`.
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.

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

Comment on lines -1145 to +1155
@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:
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.

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`.

@@ -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?

"""Creates a partial :class:`Webhook` from a webhook URL.
self, url: str, *, session: aiohttp.ClientSession, bot_token: Optional[str] = None
) -> PartialWebhook:
"""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.
``|

@@ -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?

Comment on lines +1220 to +1222
"""Fetches the current PartialSyncWebhook to get a full webhook.

This could be used to get a full webhook from a partial webhook.
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 PartialSyncWebhook to get a full webhook.
This could be used to get a full webhook from a partial webhook.
"""Fetch the full :class:`SyncWebhook` from this partial webhook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Includes breaking changes to code/packaging s: needs review Issue/PR is awaiting reviews t: enhancement New feature t: refactor/typing/lint Refactors, typing changes and/or linting changes
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

new classes: PartialWebhook and possible PartialSyncWebhook
4 participants