Skip to content

Conversation

@bramdelta
Copy link
Contributor

@bramdelta bramdelta commented Mar 9, 2025

  • Add a function for getComments
  • Add this to the README
  • Add unit tests for it

Resolves: #107

A few things:

  • I couldn't figure out how to run the VitePress site locally to verify the docs looked good, is that in another repo I should clone?
  • I apologize for my commit messages, but it seems like they're squashed anyway
  • Is the way I implemented it "good"? I was considering making a "base" getComments function that the other 3 simply used, but wasn't sure if that would introduce too much complexity, and it was consistent with the Kotlin implementation. Furthermore, is it being a new top-level comments module the right way to do it, or should that have been split up per-scope, like moving getCommentsOnAchievementWall to the achievement module?

Edit: Just saw this PR to add sort order to comment retrieval: RetroAchievements/RAWeb#3156
Maybe this PR should wait until that's done, so it can be updated to also allow sort ordering?

@wescopeland
Copy link
Member

wescopeland commented Mar 9, 2025

Hi @bramdelta 👋

Thanks for your opening your first PR into @retroachievements/api! :-)

For this first pass I'll just stick to architecture stuff. I couldn't help but notice that right now the comments stuff is exposed as multiple separate functions for consumers to use. We want to try to keep a 1:1 mapping between api-js and RAWeb, both for maintainability and backwards compatibility purposes. Ideally, api-js is as thin of wrapper as possible, with limited abstractions.

A good example of this is getTicketData() and how it maps to API_GetTicketData.php. Arguably, that endpoint should be broken into like 5 or 6 different endpoints, but api-js has no opinion on this and respects whatever it is that's in RAWeb.

I think we should condense all the comments functions down into a single function, ie: getComments().

Don't worry too much about #3156. I'm not sure what's happening with that PR presently.

@bramdelta
Copy link
Contributor Author

@wescopeland They're now all merged into a single getComments function. Since type is a reserveword I opted for kind to specify if it was an achievement or game. I also made it throw an error if a developer were to attempt to get a game/achievement's comments without specifying kind in their call, as well as a test for this.

@wescopeland
Copy link
Member

Cool! I should have time tomorrow to sit down and do a review.

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.

Looks good - just a few quick things to work through and I think we'll be all set.

@wescopeland
Copy link
Member

I couldn't figure out how to run the VitePress site locally to verify the docs looked good, is that in another repo I should clone?

Ah, apologies for the confusion here. This lives in https://github.com/RetroAchievements/api-docs.

@bramdelta
Copy link
Contributor Author

@wescopeland Other than the comment above, I think everything is resolved now. Thank you for the review! I'm still getting the hang of typescript I'm afraid

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. Thanks!

@wescopeland wescopeland merged commit 3fc2f49 into RetroAchievements:main Mar 13, 2025
1 check passed
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.

add getComments()

2 participants