Support thread member pagination
7596ff opened this issue · 8 comments
Add a new field, ThreadMember::member
, which is included when with_member
is set to true on GetThreadMembers
and GetThreadMember
requests. GetThreadMembers
now includes pagination when with_member
is true.
Blocks on #2082 being completed.
Docs PR: discord/discord-api-docs#5834
I was looking into implementing this and actually had the code in place, however I noticed an issue that didn't allow my solution to work. Currently twilight uses the Member
struct to represent guild members which includes a required guild_id
field. It seems this was included because accessing a guild member without knowledge of a guild id wasn't possible at the time. With this newest change, however, it is. I wanted to bring up some different solutions to see which one you all would prefer.
-
Make
Member::guild_id
anOption
.
This would be the most straightforward solution, but it would also be a breaking change. In terms of semantics, some users may be confused as to how a guild member has a guild_id ofNone
. -
Make a completely new structure for specifically for guild members fetched from a thread.
As far as I can tell, this change wouldn't be breaking but it would introduce some redundancy. -
Add a
guild_id
parameter to the to request option
To be specific:
let res = http
.thread_members(Id::<ChannelMarker>::new(12345678))
.with_member(true, Id::<GuildMarker>::new(17891011)) // Requires both a boolean for getting the member as well as a guild id for twilight to deserialize the member.
.await?;
Even though the API doesn't explicitly need the guild_id
, we would make it a required parameter to allow Member
to be properly serialized.
I'm also open to any other solutions or ideas.
Just my two cents on this.
According to the docs guild_id
isn't even a field on the Member object. Based on this, I feel like making guild_id
an Option would be the most consistent with Discord's API.
I could also see doing option 2. It could be something like ThreadMemberData (or MemberWithoutGuild, which would allow this to be applied to any other situations that involve member data without the guild), with a method to convert into a normal Member that accepts a guild_id.
I really dislike option 3 because that imposes a requirement for any twilight user that isn't imposed by Discord. Although it's minor, but I feel like I should be able to do anything with twilight that discord's API allows, without having to make my own requests.
I was looking into implementing this and actually had the code in place, however I noticed an issue that didn't allow my solution to work. Currently twilight uses the
Member
struct to represent guild members which includes a requiredguild_id
field. It seems this was included because accessing a guild member without knowledge of a guild id wasn't possible at the time. With this newest change, however, it is. I wanted to bring up some different solutions to see which one you all would prefer.1. Make `Member::guild_id` an `Option`. This would be the most straightforward solution, but it would also be a breaking change. In terms of semantics, some users may be confused as to how a guild member has a guild_id of `None`. 2. Make a completely new structure for specifically for guild members fetched from a thread. As far as I can tell, this change wouldn't be breaking but it would introduce some redundancy. 3. Add a `guild_id` parameter to the to request option To be specific:
let res = http .thread_members(Id::<ChannelMarker>::new(12345678)) .with_member(true, Id::<GuildMarker>::new(17891011)) // Requires both a boolean for getting the member as well as a guild id for twilight to deserialize the member. .await?;Even though the API doesn't explicitly need the
guild_id
, we would make it a required parameter to allowMember
to be properly serialized.I'm also open to any other solutions or ideas.
Thanks for taking this on. This is a problem that was encountered with other HTTP requests that work with members, so we can use their solutions here. You can set the guild ID for the response when converting the request builder into a Future, which will be used to inject the guild ID into member and member list bodies:
twilight/twilight-http/src/request/guild/member/get_guild_members.rs
Lines 98 to 117 in 85bc21e
Thanks for taking this on. This is a problem that was encountered with other HTTP requests that work with members, so we can use their solutions here. You can set the guild ID for the response when converting the request builder into a Future, which will be used to inject the guild ID into member and member list bodies:
twilight/twilight-http/src/request/guild/member/get_guild_members.rs
Lines 98 to 117 in 85bc21e
Hi, so I when I was implementing this feature I saw this done for other http routes. But unless I'm missing something I believe this can't be done like it is in GetGuildMembers
. Iirc all endpoints/events (besides getting thread members) that return guild members always share knowledge of the guild_id
, which can then be injected into the model later on. In this instance of fetching thread members, both the user initiating the request and the api response don't give a guild_id
. As a result another solution would need to be put in place to correct for this. (Again, I'm new to this codebase so correct me here if needed).
Hi, so I when I was implementing this feature I saw this done for other http routes. But unless I'm missing something I believe this can't be done like it is in GetGuildMembers. Iirc all endpoints/events (besides getting thread members) that return guild members always share knowledge of the guild_id, which can then be injected into the model later on. In this instance of fetching thread members, both the user initiating the request and the api response don't give a guild_id. As a result another solution would need to be put in place to correct for this. (Again, I'm new to this codebase so correct me here if needed).
Hm, you're correct, we don't know the guild ID, I incorrectly assumed the endpoint parameters. We may end up having to make guild_id
on Member be optional, or remove it entirely. We'll have to discuss this and get back to this issue on how to approach it soon.
Blocks on #2082 being completed
This is no longer blocked