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.

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

Prefer static_format over format with static assets #60

Merged
merged 1 commit into from Sep 16, 2021

Conversation

AstreaTSS
Copy link
Contributor

Summary

In short: something like Asset.replace(format='gif',static_format='png') would break if it encountered a static asset, as static avatars cannot be stored in a GIF format and d.py preferred to use the format format vs. the static_format format, even with static assets. According to Danny, this was an intentional feature, but I personally do not see how keeping the old behavior would be beneficial to anyone at all.
Link to old issue: #7345

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@Middledot
Copy link
Member

When converting static images to gifs, using it in embeds doesn't render the image, using it in attachments gives a http error, and visiting the asset url gives nothing, so I don't see this as completely needed

@AstreaTSS
Copy link
Contributor Author

AstreaTSS commented Sep 9, 2021

Wait, what? The PR doesn't convert static images to GIFs - it converts GIFs to GIFs and static images to static images if a format and static_format are provided instead of erroring out.

@Middledot
Copy link
Member

Middledot commented Sep 9, 2021

It appears to convert static images to gifs:
image
This fix is supposed to check if the asset is static and if yes, prefer the static_format param right?

@AstreaTSS
Copy link
Contributor Author

Okay, I probably should clarify what this does since the title is a tiny bit misleading.

If .replace is provided with both a static_format and a format...

  • If the asset is animated (handled way earlier in the program, so if it thinks the icon is still animated then that's a problem with Asset, not my PR), then it'll automatically use format regardless.
  • If the asset is not animated, check to see if static_format isn't provided. If it isn't, use format's format (mainly because this was the simplest way to achieve what the title does without changing too many lines of code and/or being unreadable code).
  • Otherwise, use static_format's format.

If something's wrong with that flow, I'm honestly not sure what (besides the animated check being bugged, maybe).

Copy link
Member

@Middledot Middledot left a comment

Choose a reason for hiding this comment

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

It turns out I was using partly outdated code, so it actually works

@Middledot Middledot merged commit 5027096 into Pycord-Development:master Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants