-
-
Notifications
You must be signed in to change notification settings - Fork 4k
fix(guildChannel): better channel editing #11072
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
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 don't know whether I like the setters in the object, but I suppose it works for this use-case 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.
Aside from @SpaceEEC's comment, the rest looks good to me
); | ||
} | ||
|
||
if (snakeCaseBody.default_reaction_emoji?.id) { |
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.
Any reason for checking id
specifically here? Passing name
wouldn't work this way.
if (snakeCaseBody.default_reaction_emoji?.id) { | |
if (snakeCaseBody.default_reaction_emoji) { |
If there is a reason for the id
check then we must also check for name
.
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.
sorry, I could be completely off base here since I haven't read everything/the types, but wouldn't your diff cause trouble if {}
was passed?
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.
good callout, I forgot emoji's can be name only. I'll add || name to it.
and @didinele no, {} would pass through in the body. Though speaking of types, I didn't add a | RESTPatchAPIChannelJSONBody to the types for this method, but I theoretically could at this point....should I?
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.
Is that the direction we foresee the discord.js library going in (i.e., allowing passing raw payloads everywhere appropriate)?
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 we should allow it (to better support new features that don't have real support yet), but I don't know that we should actually document it that way. It might be something we mention somewhere once, but I don't think we should specifically call it out except maybe types...my opinion will probably change once the lib is actually ts native and the underlying support for raw data is way better.
ce45fd2
to
077462f
Compare
toSnakeCase(options), | ||
); | ||
|
||
// This overwrites a passed snake_case version if a camelCase version OR `parent` is passed |
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.
BTW, seems that in toSnakeCase()
, if there are conflicting keys, the last one (by key order) takes precedence. i.e., if both snake case and camel case versions of a key are passed, the later one overwrites the earlier.
This won't be the case for these ones, where camel case will take precedence. Not sure if we should do something about it (maybe tweaking toSnakeCase to prefer the camel case version?..) or just let it be
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 I had set a precedent somewhere to prefer camel case when passsed which is why I did it this way.
I'm not exactly sure if I want to change that function in this PR, but I think it would make sense to because it would be more deterministic that way.
Please describe the changes this PR makes and why it should be merged:
Changes how channel edits are handled to allow sublimits to be handled properly within /rest.
Byproduct is channel editing is much more robust, allows raw api data, and can edit uncached channels now.
Supercedes #11000
Status and versioning classification: