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

Implement the ChampionByKey function and fix a bug related to fetching the champion data from spectator participant. #70

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Nico-Mayer
Copy link
Contributor

This pull request adds a function to retrieve champion data by passing the champion key as a function parameter.
This change addresses an issue with the current implementation of CurrentGameParticipant.GetChampion, which internally calls client.GetChampionById.
Although this might initially seem correct, it is problematic due to the misleading naming of the fields in Data Dragon. The championId should be a string representing the code name of the champion, while the returned value in the documentation is the champion key as a number.

image

This update corrects this behavior, ensuring that champions are returned correctly.

…riot data dragon field, this should fix the GetChampion func for spectator.
@@ -608,7 +608,7 @@ type CurrentGameParticipant struct {

// GetChampion returns the champion played by this participant
func (p *CurrentGameParticipant) GetChampion(client *datadragon.Client) (datadragon.ChampionDataExtended, error) {
return client.GetChampionByID(strconv.Itoa(p.ChampionID))
return client.GetChampionByKey(strconv.Itoa(p.ChampionID))
Copy link
Owner

Choose a reason for hiding this comment

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

let's add a comment on CurrentGameParticipant.ChampionID, describing that the value is actually the champion key. Also, I feel like this is a bug in the API :D

@Nico-Mayer
Copy link
Contributor Author

Nico-Mayer commented Jul 17, 2024

@KnutZuidema Yea i also think this is a bug or just bad naming in the riot api.

What kind of comment would you want to add here to make it clear? :)

@KnutZuidema
Copy link
Owner

@Nico-Mayer just that the ChampionID field is also the champion key in some use cases.

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.

2 participants