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

refactor(model, http, cache)!: remove guild_id from members #2083

Merged

Conversation

suneettipirneni
Copy link
Member

@suneettipirneni suneettipirneni commented Jan 20, 2023

Resolves #2082

In addition to the titular change, this PR removes the following structures:

  • MemberIntermediate
  • MemberListBody
  • MemberBody
  • MemberListDeserializer
  • MemberDeserializer

Other changes:

  • For gateway events, like MEMBER_ADD the guild_id is attached to the event tuple.
  • cache-inmemory's member_roles permission helper now takes another required parameter for the guild id.

@github-actions github-actions bot added c-cache Affects the cache crate c-http Affects the http crate c-model Affects the model crate t-refactor Refactors APIs or code. labels Jan 20, 2023
@vilgotf vilgotf linked an issue Jan 20, 2023 that may be closed by this pull request
@vilgotf vilgotf changed the title refactor(model): remove guild_id from members refactor(model)!: remove guild_id from members Jan 20, 2023
@github-actions github-actions bot added the m-breaking change Breaks the public API. label Jan 20, 2023
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.

This is great! Some notes to make it event greater

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.

This review excludes the inmemory cache changes (which require extra scrutiny). Looks good overall, just some small bits left

twilight-model/src/guild/member.rs Outdated Show resolved Hide resolved
twilight-model/src/guild/mod.rs Show resolved Hide resolved
twilight-model/src/gateway/payload/incoming/member_add.rs Outdated Show resolved Hide resolved
twilight-http/src/response/mod.rs Outdated Show resolved Hide resolved
twilight-http/src/response/mod.rs Outdated Show resolved Hide resolved
twilight-http/src/response/future.rs Outdated Show resolved Hide resolved
twilight-http/src/response/future.rs Show resolved Hide resolved
twilight-http/src/response/mod.rs Outdated Show resolved Hide resolved
twilight-model/src/gateway/payload/incoming/member_add.rs Outdated Show resolved Hide resolved
twilight-model/src/guild/mod.rs Outdated Show resolved Hide resolved
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.

Everything except for the cache (which I've not reviewed) LGTM!

@suneettipirneni suneettipirneni changed the title refactor(model)!: remove guild_id from members refactor(model, http, cache)!: remove guild_id from members Jan 23, 2023
Copy link
Member

@zeylahellyer zeylahellyer left a comment

Choose a reason for hiding this comment

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

I've got a question about the definition and how it works

@zeylahellyer
Copy link
Member

Manually ran this and received some member add events and it works as expected

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.

Looks good with just one minor exception (this time I reviewed the changes to the cache too 😉)

Comment on lines 15 to 27
impl Deref for MemberAdd {
type Target = Member;

fn deref(&self) -> &Self::Target {
&self.0
&self.member
}
}

impl DerefMut for MemberAdd {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
&mut self.member
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is confusing and I don't think we do this for any other struct with multiple items. Should it really be preserved? cc @zeylahellyer

Copy link
Member

@zeylahellyer zeylahellyer Jan 27, 2023

Choose a reason for hiding this comment

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

It may technically not be idiomatic, but we should avoid breaking even more here. We can iterate on it if needed in the next major version

@zeylahellyer zeylahellyer self-assigned this Feb 4, 2023
@zeylahellyer zeylahellyer merged commit 4dcd099 into twilight-rs:next Feb 5, 2023
@zeylahellyer
Copy link
Member

Thanks for the PR!

zeylahellyer added a commit that referenced this pull request Feb 5, 2023
Remove a link to the `member` module from the `guild` module-level
documentation. This link should have been removed in pull request #2083
but was missed.
zeylahellyer added a commit that referenced this pull request Feb 5, 2023
Remove a link to the `member` module from the `guild` module-level
documentation. This link should have been removed in pull request #2083
but was missed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-cache Affects the cache crate c-http Affects the http crate c-model Affects the model crate m-breaking change Breaks the public API. t-refactor Refactors APIs or code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Member::guild_id field
5 participants