search

simmsb / calamity

A library for writing discord bots in haskell
https://hackage.haskell.org/package/calamity
MIT License
112 stars 14 forks source link

issues with the `avatar` field in User/Member models #54

Closed ribosomerocker closed 1 year ago

ribosomerocker commented 2 years ago

Hello! This is a fantastic library that's engineered very nicely, great job! Though, I've found some weird implementation details here that could improve the library if changed to work well. Though, this probably won't be backwards compatible.. but I think any other architecture would still be good, as long as you can use the avatar field easily.

Firstly, the major issue; the avatar field in User (I'm unsure if it's the same for Member, but it most likely is) is just a hash. I understand the reasoning for it and that hash could, in some very rare situations, be useful by itself alone... but it is pretty much useless in the form that it is currently. I believe that the one we have currently should be made into an avatarHash field, and a new one named avatar that results in a link to the avatar of the user. There may be the need to have some sane defaults, like automatically selecting the size to be the biggest one by default, using the PNG format by default (and having an option to change both of those, it doesn't need to be in the avatar field, maybe a getAvatar function). But it boosts how useful it is.

I know that the avatar field of a member is a hash in the original API, but the original API is mostly used with other wrappers for a reason; to have a higher level interface.

But also, another thing; there's no reason for the avatar to be a Maybe Text. Technically, every single account in discord has an avatar. And yes, they can use the default avatars by not setting one or removing them, but it'd make everything more seamless if the library accounted for that by itself. You're able to make it a Text while providing more functionality.

MorrowM commented 2 years ago

A different idea:

Have Avatar be an opaque newtype around Maybe Text and have that be the avatar field, storing the hash. Then provide functions such as avatarHash :: Avatar -> Text, avatarURI :: Avatar -> URI, isDefaultAvatar :: Avatar -> Bool and perhaps even GetAvatarImage :: ImageFormat -> Avatar -> UserRequest ByteString.

I don't know the exact details about how avatars and default avatars work, but something along these lines is what I'd go for.

ribosomerocker commented 2 years ago

I completely agree! I think such high level functions to get information about avatars would be very useful, and they're also implemented by the other libraries. +1 for your model.

simmsb commented 2 years ago

After making a start on this I've realised it might be useful to wrap this all into a typeclass that supports fetching many different types of assets from the cdn (here's some of them)

So something like:

fetchAsset :: CDNAsset a => a -> ByteString
fetchAsset = ...

class CDNAsset a where
    assetURL :: a -> Req.Url 'Req.Https  -- or maybe just Text?

instance CDNAsset (Partial Emoji) where
    type AssetType (Partial Emoji) = ByteString

    assetURL emoji = Req.https "cdn.discordapp.com" /: "emojis" /~ (emoji ^. #id <> ".png")
ribosomerocker commented 2 years ago

That sounds like a very useful addition! I think it's very good.