docs(structures): update DataTemplate docs#11388
docs(structures): update DataTemplate docs#11388AsadHumayun wants to merge 2 commits intodiscordjs:mainfrom
Conversation
AsadHumayun
requested review from
Qjuh,
almeidx,
ckohen,
iCrawl and
vladfrangu
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis PR adds documentation remarks to DataTemplate JSDoc comments across multiple structure classes, explaining default values and how to override them using Object.defineProperties. No functional or behavioral changes. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/structures/src/users/User.ts`:
- Around line 15-21: The JSDoc for User.DataTemplate claims "This template has
defaults" but User.DataTemplate is an empty object; update the comment to
reflect there are no defaults or remove the remarks block entirely. Locate the
static property User.DataTemplate in the User class and either change the
remarks text to say there are no defaults (or that defaults live elsewhere) or
delete the paragraph about using Object.defineProperties/overriding defaults so
the documentation matches the actual empty Partial<APIUser> value.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
packages/structures/src/channels/mixins/ChannelPinMixin.tspackages/structures/src/messages/Message.tspackages/structures/src/messages/MessageCall.tspackages/structures/src/messages/Reaction.tspackages/structures/src/messages/embeds/Embed.tspackages/structures/src/users/User.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (5)
packages/structures/src/messages/embeds/Embed.ts (1)
14-22: LGTM!The documentation accurately reflects the
DataTemplatebehavior. The template has a default setter fortimestamp, and the remarks correctly explain how to extend or override it.packages/structures/src/messages/MessageCall.ts (1)
11-19: LGTM!The documentation correctly describes the
DataTemplatewhich has a default setter forended_timestamp. The remarks about usingObject.definePropertiesto extend defaults are appropriate.packages/structures/src/messages/Reaction.ts (1)
13-21: LGTM!The documentation is consistent with the actual
DataTemplateimplementation, which has a default setter forburst_colors.packages/structures/src/channels/mixins/ChannelPinMixin.ts (1)
17-27: LGTM!The documentation accurately reflects the
DataTemplatewhich contains a default setter forlast_pin_timestamp. The remarks are helpful for developers extending this mixin.packages/structures/src/messages/Message.ts (1)
22-31: LGTM!The documentation correctly describes the
DataTemplatewhich has default setters for bothtimestampandedited_timestamp. The remarks about extending vs. overriding defaults are clear and accurate.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11388 +/- ##
==========================================
- Coverage 32.43% 32.26% -0.17%
==========================================
Files 369 371 +2
Lines 13616 13691 +75
Branches 1068 1078 +10
==========================================
+ Hits 4416 4418 +2
- Misses 9065 9138 +73
Partials 135 135
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
DataTemplate is readonly by design, meaning you cannot directly set the property to another object, to avoid accidentally overwriting the defaults. |
|
Thanks for getting back to me. Considering what you've pointed out, I see it wouldn't be a good idea to suggest setting the value directly as defaults should realistically not be overridden. I'll go ahead and remove this part of the comment: Is this something that I should bring up with the wider structures team? It seems that this is/was the convention and is present in other files as well, at the moment (eg the Invite structure). Let me know if you think this looks good or other teams should be notified. |
|
I missed the change to readonly in the initial PR. I had originally not made them readonly so that consumers could actually overwrite completely, but overwritting individual properties in the same way you add properties seems better. The docs change was missed during this switch, you could update the rest of the docs as well. |
Jiralite
changed the title
|
Just to confirm we're both on the same page, is this the kind of final doc you're expecting? /**
* The template used for removing data from the raw data stored for each team.
*
* @remarks DataTemplate is readonly by design, meaning you cannot directly set the property to another object,
* to avoid accidentally overwriting the defaults.
*/
public static override readonly DataTemplate: Partial<APITeam> = {};(along with removing any mentions of "To override the defaults, set this value directly.") Since there's quite a few structures PRs in progress at the moment, I'm thinking of keeping this PR as draft and just setting them here at once, once they get merged. This saves from having to update each PR individually, though do let me know if you would prefer an alternative approach. Happy to adapt. |
|
I think keeping mention of Object.defineProperties is a good idea, but otherwise yes |
| * The template used for removing data from the raw data stored for each Channel. | ||
| * | ||
| * @remarks This template has defaults, if you want to remove additional data and keep the defaults, | ||
| * use `Object.defineProperties`. To override the defaults, set this value directly. |
There was a problem hiding this comment.
| * use `Object.defineProperties`. To override the defaults, set this value directly. | |
| * use `Object.defineProperties`. |
| * The template used for removing data from the raw data stored for each Embed. | ||
| * | ||
| * @remarks This template has defaults, if you want to remove additional data and keep the defaults, | ||
| * use `Object.defineProperties`. To override the defaults, set this value directly. |
There was a problem hiding this comment.
| * use `Object.defineProperties`. To override the defaults, set this value directly. | |
| * use `Object.defineProperties`. |
| * The template used for removing data from the raw data stored for each Message. | ||
| * | ||
| * @remarks This template has defaults, if you want to remove additional data and keep the defaults, | ||
| * use `Object.defineProperties`. To override the defaults, set this value directly. |
There was a problem hiding this comment.
| * use `Object.defineProperties`. To override the defaults, set this value directly. | |
| * use `Object.defineProperties`. |
| * The template used for removing data from the raw data stored for each MessageCall. | ||
| * | ||
| * @remarks This template has defaults, if you want to remove additional data and keep the defaults, | ||
| * use `Object.defineProperties`. To override the defaults, set this value directly. |
There was a problem hiding this comment.
| * use `Object.defineProperties`. To override the defaults, set this value directly. | |
| * use `Object.defineProperties`. |
| * The template used for removing data from the raw data stored for each Reaction. | ||
| * | ||
| * @remarks This template has defaults, if you want to remove additional data and keep the defaults, | ||
| * use `Object.defineProperties`. To override the defaults, set this value directly. |
There was a problem hiding this comment.
| * use `Object.defineProperties`. To override the defaults, set this value directly. | |
| * use `Object.defineProperties`. |
4 participants
This PR will correct comments on constructors of structures that contain defaults.
No code changes are introduced in this PR.