-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
refactor(model, http, cache)!: remove guild_id from members #2083
Conversation
22957bb
to
12d066f
Compare
There was a problem hiding this 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
bc201f8
to
b503ef1
Compare
There was a problem hiding this 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
fd964c5
to
eced9f0
Compare
df94638
to
b5e7ce6
Compare
There was a problem hiding this 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!
There was a problem hiding this 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
Manually ran this and received some member add events and it works as expected |
There was a problem hiding this 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 😉)
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 | ||
} | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Thanks for the PR! |
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.
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.
Resolves #2082
In addition to the titular change, this PR removes the following structures:
MemberIntermediate
MemberListBody
MemberBody
MemberListDeserializer
MemberDeserializer
Other changes:
MEMBER_ADD
theguild_id
is attached to the event tuple.cache-inmemory
'smember_roles
permission helper now takes another required parameter for the guild id.