-
Notifications
You must be signed in to change notification settings - Fork 137
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
feat(member): implement member banners #1204
base: master
Are you sure you want to change the base?
Conversation
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.
other than those two comments, looks good to me! seems pretty straightforward.
@@ -619,6 +621,17 @@ def guild_avatar(self) -> Optional[Asset]: | |||
return None | |||
return Asset._from_guild_avatar(self._state, self.guild.id, self.id, self._avatar) | |||
|
|||
@property | |||
def guild_banner(self) -> Optional[Asset]: |
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.
a display_banner
field would be a good QOL addition, which also keeps it in line with avatars and decorations
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.
This has no reason to exist, as it is non-functional, as the API only returns the user banners on fetched users.
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.
The gateway doesn't send user banners, but REST does, so this works fine with guild.fetch_member
which also returns user data for the requested member.
The caveat here is that the user can't be cached yet, since store_user
will prefer the cached user (without banner) over the passed user data (with banner):
Lines 328 to 330 in 93de3c8
if user_data is None: | |
user_data = cast("MemberWithUserPayload", data)["user"] | |
self._user: User = state.store_user(user_data) |
Improving this is out of scope here, but
display_banner
isn't completely useless either way.
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 don't like how it functions, given its a bit of a footgun in its current state.
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.
given its a bit of a footgun in its current state
hmm, fair enough; it is somewhat inconsistent.
In that case, I suppose display_banner
would be better suited as a TODO
for the future, once the MemberWithUserPayload
handling improves a bit. Bit unfortunate, but you're right.
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.
lgtm, thank you!
Co-authored-by: shiftinv <[email protected]> Signed-off-by: Snipy7374 <[email protected]>
@@ -619,6 +621,17 @@ def guild_avatar(self) -> Optional[Asset]: | |||
return None | |||
return Asset._from_guild_avatar(self._state, self.guild.id, self.id, self._avatar) | |||
|
|||
@property | |||
def guild_banner(self) -> Optional[Asset]: |
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.
This has no reason to exist, as it is non-functional, as the API only returns the user banners on fetched users.
Summary
Note that this is still untested as the feature is not rolled out yet.
Checklist
pdm lint
pdm pyright