feat(assets): fix animated asset extensions and add size/extension property - #3294
feat(assets): fix animated asset extensions and add size/extension property#3294SilberGecko6917 wants to merge 17 commits into
Conversation
|
Thanks for opening this pull request! This pull request can be checked-out with: git fetch origin pull/3294/head:pr-3294
git checkout pr-3294This pull request can be installed with: pip install git+https://github.com/Pycord-Development/pycord@refs/pull/3294/head |
|
I have the feeling that this PR is changing too much, but haven't had the time to take a look in depth. I will check it out asap. |
NeloBlivion
left a comment
NeloBlivion
left a comment
There was a problem hiding this comment.
Leaving my own review; since you added the size and extension kwargs, size and format should be omitted from any url variables and instead should be handled in the url @property for rendering. It would be ideal to implement urllib to handle the animated and size params.
Paillat-dev
left a comment
Paillat-dev
left a comment
There was a problem hiding this comment.
There is a naming confusion between format and extension. Let's use one consistently.
Has to be extension to make any sense imho |
Soheab
added
priority: medium
|
@SilberGecko6917 Hello, please address the change requests so we can merge this soon |
I don't see any reason for using |
SilberGecko6917
changed the title
| url: str, | ||
| key: str, | ||
| animated: bool = False, | ||
| extension: str | None = MISSING, |
There was a problem hiding this comment.
Why ae you using MISSING here ?
There was a problem hiding this comment.
for the soundboard because it doesn't have an extension.
I don't know if this is the right use but it seemed like a good approach to me
| query: dict[str, str] = {} | ||
| if self._size is not None: | ||
| query["size"] = str(self._size) | ||
| if self._animated and self._extension == "webp": |
There was a problem hiding this comment.
Would this mean that something that has animated to true but extension not to webm it would silently fail to non animated without warning ?
There was a problem hiding this comment.
we only need animated=true for webp
There was a problem hiding this comment.
Sending it with a non-animated WebP won't do anything. So it could in theory always set it 🤷
There was a problem hiding this comment.
Better not unnecessarily pass something when not needed
There was a problem hiding this comment.
yes. please dont pass it if its not supported
Signed-off-by: SilberGecko6917 <109872285+SilberGecko6917@users.noreply.github.com>
8 participants
Summary
This PR changes the
.gifasset extention for avatars, banners and other images to the.webpextention.It also adds two new propertys (
size,extension).https://docs.discord.com/developers/reference#image-formatting
I noticed that if a user had an animated profile picture, the URL of the image returned a
415 Unsupported Media Typeerror. For example, here:https://cdn.discordapp.com/guilds/1010915072694046794/users/851762178532311050/avatars/a_8a63bc5a3a227954876eb04778006c13.gif?size=1024
but with webp it works:
https://cdn.discordapp.com/guilds/1010915072694046794/users/851762178532311050/avatars/a_8a63bc5a3a227954876eb04778006c13.webp?animated=true&size=1024
I used claude (https://claude.ai/) to check over my code and get some help with how i could fix the issue.
Information
examples, ...).
Checklist
type: ignorecomments were used, a comment is also left explaining why.