-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Add ability to use primary guild (clan) data for users #10211
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
Conversation
|
There is already a PR for this: #10190 |
dolfies
left a comment
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.
the clan field is deprecated, this should use primary_guild (and the naming should prolly reflect that as clans are a dead concept)
Co-authored-by: DA344 <[email protected]>
Forgot to add this one Co-authored-by: DA344 <[email protected]>
… correct fix, but it works
Co-authored-by: dolfies <[email protected]>
Co-authored-by: dolfies <[email protected]>
Any idea how often discord updates their documentation? Latest pull in discord's api docs for tags was opened 2nd May 2024 and has not been updated in the past 3 weeks discord/discord-api-docs#6836 |
|
Also the api does seem to be stable. From discord/discord-api-docs#6836 (comment) (19th May 2024) you can see that they still use the same fields of |
|
You'll need to wait for Discord to just merge the documentation of it, and although it looks stable doesn't mean it will always be, rn is not documented so they have an excuse to (although even documented they do) break the field at any point. |
Yea I get that, but tags have been rolled out to all servers now. The one change discord did was the field name from +In the unlikely event they do, I would be more than happy to fix/patch/whatever the code |
|
Still, docs must be merged first |
|
Generally speaking I don't merge features that are undocumented because we've been bitten by it in the past (most recently, voice channel statuses) and it's not worth the upkeep. |
|
The API docs have been merged per discord/discord-api-docs@a1bac5f ! |
DA-344
left a comment
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.
The route has changed
Co-authored-by: DA344 <[email protected]>
|
Just double checked the docs and everything seems good to me |
Soheab
left a comment
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.
LGMT overall.
discord/primary_guild.py
Outdated
| payload: PrimaryGuildPayload = {"identity_guild_id": None, "identity_enabled": False, "tag": None, "badge": None} | ||
| return cls(state=state, data=payload) |
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.
You don't really need to define each key here; an empty dict would work too since the _update method makes everything default to None regardless.
I guess it's better to be explicit, but I’m not sure.
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 agree here.
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.
Changed it to set only identity_enabled to False, as None has a specific meaning
discord/primary_guild.py
Outdated
| payload: PrimaryGuildPayload = {"identity_guild_id": None, "identity_enabled": False, "tag": None, "badge": None} | ||
| return cls(state=state, data=payload) |
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 agree here.
|
Sorry for delays, was on holiday |
Rapptz
left a comment
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.
LGTM. Thanks
Summary
Checklist