Skip to content

Conversation

@ioslife
Copy link
Contributor

@ioslife ioslife commented Jan 30, 2025

Alter the GetComments endpoint from #2552 to add a sortOrder query param

/*
*  API_GetComments - returns the comments associated to a game or achievement
*    i : game or achievement id or username
*    t : 1 = game, 2 = achievement, 3 = user
*    o : offset - number of entries to skip (default: 0)
*    c : count - number of entries to return (default: 100, max: 500)
*    s : sortOrder - sort comments. 'submitted' = ascending, '-submitted' = descending (default: 'submitted')
*
*  int         Count                       number of comment records returned in the response
*  int         Total                       number of comment records the game/achievement/user actually has overall
*  array       Results
*   object      [value]
*    int         User                      username of the commenter
*    string      Submitted                 date time the comment was submitted
*    string      CommentText               text of the comment
*/

@ioslife
Copy link
Contributor Author

ioslife commented Jan 30, 2025

FYI – I am open to a different query letter here

* t : 1 = game, 2 = achievement, 3 = user
* o : offset - number of entries to skip (default: 0)
* c : count - number of entries to return (default: 100, max: 500)
* s : sortOrder - sort comments. 0 = ascending, 1 = descending (default: 0)
Copy link
Member

Choose a reason for hiding this comment

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

This could be too ambiguous in terms of what the orderBy() is. I'm also thinking integers as possible values may be inflexible if this pattern is extended to other API endpoints.

I think we may want to change this to use JSON:API-like enum values, ie: createdAt and -createdAt.

Copy link
Member

Choose a reason for hiding this comment

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

We do have a few public APIs where filter parameters are effectively booleans (0=all,1=filtered), and some that are direct mapping of enums (3=core,5=unofficial).

But as this is not an enum, and it's not being implemented as a boolean (s: sort descending (0=no,1=yes)), I agree that it makes sense to have it be more free-form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I will check on this next week. Thanks for looking at it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, it is I! Finally making finishing this a priority, apologies for the PR and run.

I just pushed a commit that accepts submitted and -submitted as the s param. I am willing to add others to the sortOptions enum, but wanted to verify this is the pattern you were thinking @wescopeland.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I think so. I would rename it to sort to align with some of our other paths like: https://retroachievements.org/games?sort=-achievementsPublished

* t : 1 = game, 2 = achievement, 3 = user
* o : offset - number of entries to skip (default: 0)
* c : count - number of entries to return (default: 100, max: 500)
* s : sortOrder - sort comments. 0 = ascending, 1 = descending (default: 0)
Copy link
Member

Choose a reason for hiding this comment

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

We do have a few public APIs where filter parameters are effectively booleans (0=all,1=filtered), and some that are direct mapping of enums (3=core,5=unofficial).

But as this is not an enum, and it's not being implemented as a boolean (s: sort descending (0=no,1=yes)), I agree that it makes sense to have it be more free-form.

@wescopeland wescopeland marked this pull request as ready for review April 1, 2025 21:18
Copy link
Member

@wescopeland wescopeland left a comment

Choose a reason for hiding this comment

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

LGTM - we just need an accompanying docs PR before we can ship this in a release.

@ioslife
Copy link
Contributor Author

ioslife commented Apr 14, 2025

LGTM - we just need an accompanying docs PR before we can ship this in a release.

Awesome, thanks! Will push docs PR tomorrow.

@ioslife
Copy link
Contributor Author

ioslife commented Apr 14, 2025

@wescopeland wescopeland merged commit c031ea9 into RetroAchievements:master Apr 14, 2025
10 checks passed
@ioslife ioslife deleted the api/get-comments-descending branch April 14, 2025 19:25
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.

3 participants