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.

Sign up for GitHub

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 is_super() method to Reaction #10011

Closed
wants to merge 1 commit into from
Closed

Add is_super() method to Reaction #10011

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions discord/reaction.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,13 @@ def is_custom_emoji(self) -> bool:
""":class:`bool`: If this is a custom emoji."""
return not isinstance(self.emoji, str)

def is_super(self) -> bool:
""":class:`bool`: Whether this reaction is a super reaction.
Comment on lines +117 to +118
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be more acceptable is the following:-

Suggested change
def is_super(self) -> bool:
""":class:`bool`: Whether this reaction is a super reaction.
def has_super(self) -> bool:
""":class:`bool`: Whether this reaction has a super reaction within.

Meaning the end user can check if the Reaction has a super reaction. However one could argue we'd then also need a has_normal which doesn't read very well to me.

Copy link
Contributor

@owocado owocado Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This feels more weird meaning and readability wise. The way it is makes more sense than Reaction.has_super imo. A reaction don't have a super reaction within.


.. versionadded:: 2.5
"""
return self.normal_count == 0 and self.burst_count > 0
Copy link
Contributor

@AbstractUmbra AbstractUmbra Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Curious.
Is there a situation where normal_count AND burst_count could be > 0?
If so, wouldn't this result in a false negative?

EDIT: Just tested and you cannot add a super reaction and a normal reaction of the same emoji. Surely this check should just be like so, then:-

Suggested change
return self.normal_count == 0 and self.burst_count > 0
return self.burst_count > 0

Copy link
Contributor

Choose a reason for hiding this comment

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

You can, you just can't do it yourself. E.g. I can add emoji x, another person can add emoji x as a super reaction. The reactions from the API should still only be 1 object, with a count of 2 (one burst and one normal), even though they appear separately on the clients.

Copy link
Contributor

@AbstractUmbra AbstractUmbra Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Further testing reveals that Reactions are condensed into a single object as determined by the emoji used. Meaning if there are >=1 super reactions AND >=1 normal reactions of the same emoji, these will be the same Reaction object, rendering this PR incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, that’s annoying.

The original point of this was just to determine whether the reaction was a super one, but I didn’t realize that other users could add a normal version of the same emoji.

We could still make this work by removing the normal_count check, but that would be misleading.

I guess has_super/burst could be an alternative naming, but that’s also a bit misleading since reactions aren’t a container like said in the server.

I think it might be best to scrap this and look into better solutions like separating the classes or something at the API level.


def __eq__(self, other: object) -> bool:
return isinstance(other, self.__class__) and other.emoji == self.emoji

Expand Down
Loading