twilight-rs/twilight

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.

  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 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 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.

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 allow Member 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:

impl IntoFuture for GetGuildMembers<'_> {
type Output = Result<Response<MemberListBody>, Error>;
type IntoFuture = ResponseFuture<MemberListBody>;
fn into_future(self) -> Self::IntoFuture {
let guild_id = self.guild_id;
let http = self.http;
match self.try_into_request() {
Ok(request) => {
let mut future = http.request(request);
future.set_guild_id(guild_id);
future
}
Err(source) => ResponseFuture::error(source),
}
}
}

Somewhat related to #1364

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:

impl IntoFuture for GetGuildMembers<'_> {
type Output = Result<Response<MemberListBody>, Error>;
type IntoFuture = ResponseFuture<MemberListBody>;
fn into_future(self) -> Self::IntoFuture {
let guild_id = self.guild_id;
let http = self.http;
match self.try_into_request() {
Ok(request) => {
let mut future = http.request(request);
future.set_guild_id(guild_id);
future
}
Err(source) => ResponseFuture::error(source),
}
}
}

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