-
Notifications
You must be signed in to change notification settings - Fork 47
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
Application Emoji Support #338
base: main
Are you sure you want to change the base?
Conversation
/// <param name="applicationID">The ID of the application.</param> | ||
/// <param name="ct">The cancellation token for this operation.</param> | ||
/// <returns>A retrieval result which may or may not have succeeded.</returns> | ||
Task<Result<IReadOnlyList<IEmoji>>> ListApplicationEmojisAync |
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.
Aync
Task<Result<IEmoji>> GetGuildEmojiAsync | ||
( | ||
Task<Result<IEmoji>> GetGuildEmojiAsync( |
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.
?
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.
Probably an error with auto-formatting?
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.
Yeah, I think my Rider did that automatically
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.
Thank you for this contribution! I've pointed out some small issues (pretty much in the name of consistency.)
( | ||
$"applications/{applicationID}/emojis", | ||
b => b | ||
.AddAuditLogReason(reason) |
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.
Creating application emojis doesn't support the X-AuditLog-Reason header
$"applications/{applicationID}/emojis/{emojiID}", | ||
b => b | ||
.AddAuditLogReason(reason) | ||
.WithJson( |
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.
style: allman paranthesis (open and closing should be on their on lines)
Task<Result<IEmoji>> GetGuildEmojiAsync | ||
( | ||
Task<Result<IEmoji>> GetGuildEmojiAsync( |
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.
Probably an error with auto-formatting?
( | ||
$"applications/{applicationID}/emojis/{emojiID}", | ||
b => b | ||
.AddAuditLogReason(reason) |
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.
ditto
Tests should also be added for the new endpoints. cc @Nihlus? |
return this.RestHttpClient.GetAsync<IReadOnlyList<IEmoji>> | ||
( | ||
$"applications/{applicationID}/emojis", | ||
"items", |
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 by no means your fault - it's Discord's, somewhat, but this fails with an exception when the app doesn't have any emojis.
The input does not contain any JSON tokens. Expected the input to start with a valid JSON token, when isFinalBlock is true.
The returned JSON is in fact, correct: {"items":[]}
, but STJ just...dies.
There are still unresolved comments on this pull request. I'm working through my queue at the moment, so please address them if you'd like this code to be merged for the next release :) |
This pull request adds the additional endpoints of the recently added application emoji feature to the emoji API.