Add is_super() method to Reaction #10011
Conversation
|
|
||
| .. versionadded:: 2.5 | ||
| """ | ||
| return self.normal_count == 0 and self.burst_count > 0 |
There was a problem hiding this comment.
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:-
| return self.normal_count == 0 and self.burst_count > 0 | |
| return self.burst_count > 0 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 is_super(self) -> bool: | ||
| """:class:`bool`: Whether this reaction is a super reaction. |
There was a problem hiding this comment.
What would be more acceptable is the following:-
| 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.
There was a problem hiding this comment.
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.
|
The API combines these all so fundamentally the PR isn't correct terminology nor how it's represented. The client separates the rendering on its own but that doesn't mean they're separate objects. Since this PR doesn't make sense at an API level I'm going to just close it. |
Summary
This PR adds an easy to use helper method to determine whether a reaction is a super reaction.
Checklist