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

Add support for message snapshots used in message forwarding #9892

Closed
wants to merge 21 commits into from

Conversation

redmvc
Copy link
Contributor

@redmvc redmvc commented Jul 22, 2024

Summary

Message forwarding was introduced on July 18, 2024. Messages now have a new optional parameter, message_snapshots, which is an array of Message Snapshots containing immutable snapshots of a message being forwarded. Such messages will, additionally, always have a reference to the message being forwarded, and that reference will have type = 1.

This PR adds a MessageSnapshot class and a message_snapshots parameter to the Message class. It also adds the type parameter to the MessageReference class, in order to track whether a reference is a reply or a forward.

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • 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, ...)

Create enums and class attributes tracking the type of a message reference (default or forward)
Create a class to track a snapshot of a message
Add class MessageSnapshot to classes exported from message.py
@Soheab
Copy link
Contributor

Soheab commented Jul 22, 2024

Can't this use the existing Message class? With a potential _from_snapshot classmethod

@redmvc
Copy link
Contributor Author

redmvc commented Jul 22, 2024

Message snapshots don't have all Message information; in particular, they don't have an author.

discord/message.py Outdated Show resolved Hide resolved
discord/message.py Outdated Show resolved Hide resolved
discord/enums.py Show resolved Hide resolved
discord/message.py Outdated Show resolved Hide resolved
discord/message.py Show resolved Hide resolved
discord/message.py Outdated Show resolved Hide resolved
Copy link
Contributor

@owocado owocado left a comment

Choose a reason for hiding this comment

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

I think that "forwarded" would make more sense here (in context of users reading relevant documentation) instead of "snapshotted".

discord/message.py Outdated Show resolved Hide resolved
discord/message.py Outdated Show resolved Hide resolved
discord/message.py Outdated Show resolved Hide resolved
discord/message.py Outdated Show resolved Hide resolved
discord/message.py Outdated Show resolved Hide resolved
discord/message.py Outdated Show resolved Hide resolved
@@ -452,6 +455,92 @@ def guild_id(self) -> Optional[int]:
return self._parent.guild_id


class MessageSnapshot:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could have a cached_message attribute that, as its name indicates, has the cached message of this snapshot, if found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ever found? I tested it for a while and none of the forwards I saw had that attribute non-empty

Copy link
Contributor

@DA-344 DA-344 Jul 27, 2024

Choose a reason for hiding this comment

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

As said, if the message is found in the cache then this would be non-empty. If a user forwards a message the bot has cached then the attribute would be filled, else not.
As forwarding is just referencing a message, this would be almost the same as MessageReference.cached_message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait sorry I misunderstood. Yeah that seems like not a bad idea, though I'd need to look into how the cache works exactly

Copy link
Owner

Choose a reason for hiding this comment

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

This is a good idea, it shouldn't be a complicated add either. You already pass the connection state as a parameter but you need to bind it to make it into a property.

Copy link
Owner

@Rapptz Rapptz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, some minor nits.

@@ -452,6 +455,92 @@ def guild_id(self) -> Optional[int]:
return self._parent.guild_id


class MessageSnapshot:
"""Represents a message snapshot attached to a forwarded message.
Copy link
Owner

Choose a reason for hiding this comment

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

This needs a .. versionadded:: 2.5 into it.

@@ -452,6 +455,92 @@ def guild_id(self) -> Optional[int]:
return self._parent.guild_id


class MessageSnapshot:
Copy link
Owner

Choose a reason for hiding this comment

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

This is a good idea, it shouldn't be a complicated add either. You already pass the connection state as a parameter but you need to bind it to make it into a property.

@@ -218,6 +218,11 @@ def __str__(self) -> str:
return self.name


class MessageReferenceType(Enum):
default = 0
Copy link
Owner

Choose a reason for hiding this comment

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

This should be called reply with an alias to default. The API docs even call it REPLY and DEFAULT despite not mentioning the former. https://discord.com/developers/docs/resources/message#message-reference-types

Comment on lines +554 to +555
type: :class:`MessageReferenceType`
The type of message reference.
Copy link
Owner

Choose a reason for hiding this comment

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

Needs a versionadded.

Comment on lines +582 to +590
def __init__(
self,
*,
type: MessageReferenceType,
message_id: int,
channel_id: int,
guild_id: Optional[int] = None,
fail_if_not_exists: bool = True,
):
Copy link
Owner

Choose a reason for hiding this comment

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

MessageReference is a 'data class' in discord.py which means users can create it, making this a breaking change for those who did since they no longer have a type parameter. You should give it a default value.

Comment on lines +5169 to +5176
MessageSnapshot
~~~~~~~~~~~~~~~~~

.. attributetable:: MessageSnapshot

.. autoclass:: MessageSnapshot
:members:

Copy link
Owner

Choose a reason for hiding this comment

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

Since this is an API received object it needs to be in the model section with the constructor hidden.

@Rapptz Rapptz added the pending PR implements an in-progress or explicitly unreleased Discord feature label Aug 31, 2024
@Puncher1
Copy link
Contributor

As new PR opened on Discord's side, this is missing following fields in MessageSnapshot: stickers, sticker_items, and components.

@redmvc
Copy link
Contributor Author

redmvc commented Sep 28, 2024

Sorry, it's unlikely I'll be able to resume work on this PR anytime soon. If anybody wants to take the code, I encourage it.

@DA-344 DA-344 mentioned this pull request Sep 29, 2024
6 tasks
@Rapptz
Copy link
Owner

Rapptz commented Oct 9, 2024

Superseded by #9950

@Rapptz Rapptz closed this Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending PR implements an in-progress or explicitly unreleased Discord feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants