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

Application Emoji Support #338

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Application Emoji Support #338

wants to merge 1 commit into from

Conversation

crobati
Copy link

@crobati crobati commented Jul 30, 2024

This pull request adds the additional endpoints of the recently added application emoji feature to the emoji API.

/// <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
Copy link
Contributor

Choose a reason for hiding this comment

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

Aync

Comment on lines -59 to +141
Task<Result<IEmoji>> GetGuildEmojiAsync
(
Task<Result<IEmoji>> GetGuildEmojiAsync(
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor

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?

Copy link
Author

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

Copy link
Contributor

@VelvetToroyashi VelvetToroyashi left a 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)
Copy link
Contributor

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(
Copy link
Contributor

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)

Comment on lines -59 to +141
Task<Result<IEmoji>> GetGuildEmojiAsync
(
Task<Result<IEmoji>> GetGuildEmojiAsync(
Copy link
Contributor

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@VelvetToroyashi
Copy link
Contributor

Tests should also be added for the new endpoints. cc @Nihlus?

return this.RestHttpClient.GetAsync<IReadOnlyList<IEmoji>>
(
$"applications/{applicationID}/emojis",
"items",
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 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.

@Nihlus
Copy link
Member

Nihlus commented Sep 4, 2024

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 :)

@Nihlus Nihlus added the waiting for author The PR is awaiting an update from the author label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author The PR is awaiting an update from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants