-
Notifications
You must be signed in to change notification settings - Fork 140
feat(emoji)!: implement app emojis #1224
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
base: master
Are you sure you want to change the base?
Conversation
I marked this ready for review even though i still don't know if we should provide the ability to cache app emojis at startup, as you can see i did put a boolean argument but didn't implement any logic for it so far. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the documentation, referring the emojis as "application emojis" instead of "app emojis" would be more consistent
Co-authored-by: Victor <[email protected]> Signed-off-by: Snipy7374 <[email protected]>
Co-authored-by: Victor <[email protected]> Signed-off-by: Snipy7374 <[email protected]>
Co-authored-by: Victor <[email protected]> Signed-off-by: Snipy7374 <[email protected]>
Co-authored-by: Victor <[email protected]> Signed-off-by: Snipy7374 <[email protected]>
Co-authored-by: Victor <[email protected]> Signed-off-by: Snipy7374 <[email protected]>
Co-authored-by: Victor <[email protected]> Signed-off-by: Snipy7374 <[email protected]>
Co-authored-by: Victor <[email protected]> Signed-off-by: Snipy7374 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since .guild
/.guild_id
is changed to possibly return None
, this is a breaking change. (Rename PR title to feat!(emoji): ...
?)
Personally I'd rather see a separate class for app emojis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm strongly in favor of splitting the models into a GuildEmoji
and AppEmoji
. The current approach is pretty much saying "an Emoji can have both .guild_id
and .application_id
, one of, or neither!"
Anyhow, minor nitpicks below.
@@ -0,0 +1 @@ | |||
:attr:`Emoji.guild_id` can now be ``None`` is the emoji is owned by an application. You can use :meth:`Emoji.is_app_emoji` to check for that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:attr:`Emoji.guild_id` can now be ``None`` is the emoji is owned by an application. You can use :meth:`Emoji.is_app_emoji` to check for that. | |
:attr:`Emoji.guild_id` can now be ``None`` if the emoji is owned by an application. You can use :meth:`Emoji.is_app_emoji` to check for that. |
|
||
This can now return ``None`` if the emoji is an | ||
application owned emoji. | ||
""" | ||
# this will most likely never return None but there's a possibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is no longer saying anything useful, remove it perhaps?
if self.guild_id is None: | ||
return True | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if self.guild_id is None: | |
return True | |
return False | |
return self.guild_id is None |
If we want to split them into different classes (I had this talk with @shiftinv in the past, but at the time they said it was fine to go with this approach) we would most likely split them into |
Summary
Checklist
pdm lint
pdm pyright