twilight-rs / twilight

Powerful, flexible, and scalable ecosystem of Rust libraries for the Discord API.
https://discord.gg/twilight-rs
ISC License
652 stars 130 forks source link

Seemingly unintentional limitations to update_guild_channel_positions #2327

Closed jonastar closed 3 months ago

jonastar commented 4 months ago

I found a couple of issues with the implementation of this endpoint.

First off: the docs and the implementation don't match

    /// Modify the positions of the channels.
    ///
    /// The minimum amount of channels to modify, is a swap between two channels.
    ///
    /// This function accepts an `Iterator` of `(Id<ChannelMarker>, u64)`. It also
    /// accepts an `Iterator` of `Position`, which has extra fields.
    pub const fn update_guild_channel_positions<'a>(
        &'a self,
        guild_id: Id<GuildMarker>,
        channel_positions: &'a [Position],
    ) -> UpdateGuildChannelPositions<'a> {
        UpdateGuildChannelPositions::new(self, guild_id, channel_positions)
    }

It does not accept an iterator.

The second issue relates to the position type:

#[derive(Serialize)]
pub struct Position {
    id: Id<ChannelMarker>,
    #[serde(skip_serializing_if = "Option::is_none")]
    lock_permissions: Option<bool>,
    #[serde(skip_serializing_if = "Option::is_none")]
    parent_id: Option<Id<ChannelMarker>>,
    #[serde(skip_serializing_if = "Option::is_none")]
    position: Option<u64>,
}

impl From<(Id<ChannelMarker>, u64)> for Position {
    fn from((id, position): (Id<ChannelMarker>, u64)) -> Self {
        Self {
            id,
            lock_permissions: None,
            parent_id: None,
            position: Some(position),
        }
    }
}

The fields are all private so I can't construct this struct myself, leading me to only be able to use the included From implementation, and not use the other fields.

Erk- commented 4 months ago

Yeah that looks like a mistake, at the same time I spotted that some of those fields have to be able to be null as well so I fixed that as well. I removed the wrong part of the comment as I don't think it is needed.