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

Add new RiotID routes; Capture RiotIDGameName from MatchV5 #60

Merged

Conversation

harmonherring
Copy link
Contributor

@harmonherring harmonherring commented Jan 16, 2024

Since it seems #59 is dead I've opened this PR. It's pretty much identical besides a few minor fixes

  • Fixes a few problems that kept new endpoints from working
  • Fixes log fields named incorrectly
  • Fixes by-riot-id endpoint path
  • Captures RiotIDGameName from MatchV5 endpoint

)

// AccountClient provides methods for the account endpoints of the League of Legends API.
type AccountClient struct { //nolint:golint
Copy link
Contributor Author

@harmonherring harmonherring Jan 16, 2024

Choose a reason for hiding this comment

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

@KnutZuidema in the other PR you mentioned this should be renamed to Client. However, account.Client already exists in account/client.go so that this package conforms to the same structure in the lol package.

I can change this if you want, but if another account-family endpoint(s) is added by riot at some point it may be useful to keep this structure? Up to you

Copy link
Owner

Choose a reason for hiding this comment

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

Fair point. I would assume that the endpoint hierarchy for accounts will stay flat, so let's just implement all the actions directly on the Client type. If another set of endpoints is added later, we'll have to refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@harmonherring harmonherring marked this pull request as ready for review January 17, 2024 03:39
@harmonherring harmonherring marked this pull request as draft January 17, 2024 03:41
@harmonherring harmonherring marked this pull request as ready for review January 17, 2024 03:47
@KnutZuidema
Copy link
Owner

@harmonherring, not sure how it got in there, but the commit refactor: test response body does not belong here. You can drop it from the branch.

@Nico-Mayer
Copy link
Contributor

Riot also stated in this article that

  • /lol/summoner/v4/summoners/by-name/{summonerName}
  • /tft/summoner/v1/summoners/by-name/{summonerName}

endpoints are deprecated from now on, so it may be a good idea to include a comment on this.

@Nico-Mayer
Copy link
Contributor

@KnutZuidema are there plans to merge this MR in the near future?

@harmonherring harmonherring force-pushed the capture-game-name-and-fixes branch from 5dd1723 to 8e2cb65 Compare April 11, 2024 01:33

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
@harmonherring harmonherring force-pushed the capture-game-name-and-fixes branch from 8e2cb65 to 1bce0c4 Compare April 11, 2024 01:35

Unverified

This user has not yet uploaded their public signing key.
@harmonherring
Copy link
Contributor Author

@KnutZuidema apologies for the delay on this 😅 I've implemented the account methods directly on Client as you asked and also rebased that odd commit out of history

@harmonherring
Copy link
Contributor Author

Riot also stated in this article that

  • /lol/summoner/v4/summoners/by-name/{summonerName}
  • /tft/summoner/v1/summoners/by-name/{summonerName}

endpoints are deprecated from now on, so it may be a good idea to include a comment on this.

I agree, let's do this in a separate PR 😄

Copy link
Owner

@KnutZuidema KnutZuidema left a comment

Choose a reason for hiding this comment

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

Thanks @harmonherring 🙏🏼 LGTM

@KnutZuidema KnutZuidema merged commit d942e20 into KnutZuidema:master Apr 17, 2024
3 checks passed
@harmonherring harmonherring mentioned this pull request Apr 17, 2024
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.

None yet

4 participants