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

fix(model, http)!: fix role position update payload and allow audit log reason #2342

Merged
merged 3 commits into from
Jul 13, 2024

Conversation

suneettipirneni
Copy link
Member

Closes #2338 and adds audit log reason support for this endpoint.

@github-actions github-actions bot added c-http Affects the http crate c-model Affects the model crate t-fix Fixes a bug in the library labels May 13, 2024
@suneettipirneni suneettipirneni self-assigned this May 13, 2024
@suneettipirneni suneettipirneni requested a review from a team May 13, 2024 19:48
Copy link
Member

@vilgotf vilgotf left a comment

Choose a reason for hiding this comment

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

(Thinking out loud) We could keep the &[(Id, u64)] parameter as is, with a little magic, via a custom serializer (on a private newtype) like:

-     roles: &'a [(Id<RoleMarker>, u64)],
+     roles: Roles<'a>,
struct Roles<'a>(&'a [(Id<RoleMarker>, u64)]);

impl Serialize for Roles<'_> {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        serializer.collect_seq(self.0.map(/* tuple to RolePosition  */))
    }
}

@vilgotf vilgotf changed the title fix(model, http): fix role position update payload and allow audit log reason fix(model, http)!: fix role position update payload and allow audit log reason May 14, 2024
@github-actions github-actions bot added the m-breaking change Breaks the public API. label May 14, 2024
@Erk-
Copy link
Member

Erk- commented May 14, 2024

I think I would like the struct way more than the tuple.

@suneettipirneni suneettipirneni requested a review from vilgotf June 1, 2024 17:30
twilight-model/src/guild/role_position.rs Outdated Show resolved Hide resolved
twilight-model/src/guild/role_position.rs Outdated Show resolved Hide resolved
Co-authored-by: Tim Vilgot Mikael Fredenberg <26655508+vilgotf@users.noreply.github.com>
Copy link
Member

@AEnterprise AEnterprise left a comment

Choose a reason for hiding this comment

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

Not a huge fan of the "RolePosition" name since it contains more then just the position. Maybe "RoleMeta" or so might be better but that might then also implies it has more? not entirely sure if there is a better name

@suneettipirneni suneettipirneni merged commit 0063af4 into main Jul 13, 2024
9 checks passed
@suneettipirneni suneettipirneni deleted the fix/update-role-positions branch July 13, 2024 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-http Affects the http crate c-model Affects the model crate m-breaking change Breaks the public API. t-fix Fixes a bug in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

twilight-http: Client::update_role_positions uses wrong structure format
4 participants