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

Document role subscription system channel flags and message field #5828

Merged

Conversation

advaith1
Copy link
Contributor

@advaith1 advaith1 commented Jan 7, 2023

I missed these in #5724

btw, thanks for using a proper object in the message field instead of abusing embed fields like automod (is that gonna be changed/is that format stabilized yet?)

Copy link

@magskai magskai 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! I left one comment about making total_months_subscribed description more explicit. Thanks for documenting all this!

| ---------------------------- | --------- | --------------------------------------------------------------------- |
| role_subscription_listing_id | snowflake | the id of the sku and listing that the user is subscribed to |
| tier_name | string | the name of the tier that the user is subscribed to |
| total_months_subscribed | integer | the number of months that the user has been subscribed for |
Copy link

Choose a reason for hiding this comment

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

If you want to be extra explicit, I would say cumulative number of months (vs consecutive)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

| position? | integer | A generally increasing integer (there may be gaps or duplicates) that represents the approximate position of the message in a thread, it can be used to estimate the relative position of the message in a thread in company with `total_message_sent` on parent thread |
| role_subscription_data? | [role subscription data](#DOCS_RESOURCES_CHANNEL/role-subscription-data-object) object | data of the role subscription purchase or renewal that prompted this ROLE_SUBSCRIPTION_PURCHASE message |
Copy link
Contributor

Choose a reason for hiding this comment

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

this is correct for now, we actually wanted to give it a generic name so that we can store any role subscription data on a message in one place. Right now we're using it to show the purchase recognition message but in the future, we might use this field to store some other role subscription message data.

We can update this in the future if we use it for something else though 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

If this were changed how would an application deal with it without crashing?

Copy link
Contributor

Choose a reason for hiding this comment

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

The precedent for volatile internal fields is to prefix the name with _, similar to _trace in the READY dispatch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I mark any of the fields as optional and/or nullable?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this were changed how would an application deal with it without crashing?

what I mean is that we might add new fields to that role_subscription_data for a new message type in the future and at that point, this field wouldn't be only used for the ROLE_SUBSCRIPTION_PURCHASE message type. we definitely won't change anything in the role_subscriptions_data for the ROLE_SUBSCRIPTION_PURCHASE type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if a field is not marked as optional then libraries would assume that the field will always be present in role_subscription_data, and might error if that is not true

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, seems like my memory is failing me, looks like we didn't mark any of those fields optional on the clientside so ignore my comments

@appellation appellation merged commit 33b65c8 into discord:main Jan 10, 2023
@advaith1 advaith1 deleted the role-subscriptions-2-electric-boogaloo branch February 16, 2023 04:08
lukellmann added a commit to lukellmann/kord that referenced this pull request Mar 4, 2023
* rename GuildFeature.CreatorMonetizableProvision to
  GuildFeature.CreatorMonetizableProvisional

* deprecate GuildFeature.MonetizationEnabled, see
  discord/discord-api-docs#5724 (comment)

* add @SerialName for DiscordRoleTags.botId

* add RoleTags.isLinkedRole

* fix DiscordMessage.roleSubscriptionData nullability

* fix KDocs

see discord/discord-api-docs#5724 and
discord/discord-api-docs#5828
lukellmann added a commit to kordlib/kord that referenced this pull request Mar 5, 2023
* rename GuildFeature.CreatorMonetizableProvision to
  GuildFeature.CreatorMonetizableProvisional

* deprecate GuildFeature.MonetizationEnabled, see
  discord/discord-api-docs#5724 (comment)

* add @SerialName for DiscordRoleTags.botId

* add RoleTags.isLinkedRole

* fix DiscordMessage.roleSubscriptionData nullability

* fix KDocs

see discord/discord-api-docs#5724 and
discord/discord-api-docs#5828

Co-authored-by: NoComment <67918617+NoComment1105@users.noreply.github.com>
shaydewael pushed a commit to Jupith/discord-api-docs that referenced this pull request May 14, 2024
…scord#5828)

* Document role subscription system channel flags and message field

* cumulative
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants