search

taycaldwell / riot-api-java

Riot Games API Java Library
http://taycaldwell.github.io/riot-api-java/
Apache License 2.0
190 stars 73 forks source link

Inconsistency of championId type #74

Closed briceambrosiak closed 8 years ago

briceambrosiak commented 8 years ago

The method ChampionMastery.getChampionId() returns a long type. However, Static.Champion.getDataChampion(...) takes the id as int type (also, field could be name championId for clarity). I don't know if other differences like that exists elsewhere in the API.

taycaldwell commented 8 years ago

These types mirror the types defined in the official Riot Games API documentation.

A lot of the underlying services that power the API are owned by different teams across Riot. In the past that has been their explanation for little type and naming inconsistencies across endpoints like this.

IIRC, in the past we addressed this to make the library more consistent, but I guess we overlooked these specific endpoints. (or maybe a thread was made to ask Riot why this inconsistency exists, but we never really did anything about it on our end? idr.)

Either way, its a simple change. The GSON conversion should still work if we change the champion ID type to int instead of long in the necessary DTOs.

Linnun commented 8 years ago

Are you sure you are using the latest version of this API? ChampionMastery.getChampionId() returns an integer value since 7 months now (February 2016) (reference: https://github.com/rithms/riot-api-java/commit/a9f76b18ae8c9d2f67c29eee9a3059132da48503)

taycaldwell commented 8 years ago

Ah, I knew we changed that.

The latest available release is still 3.9.0 on GitHub (unless developers export their own JAR from the source), which was released before these changes were made.

Hm... It's been a while, maybe we should release 4.0 LOL.

Linnun commented 8 years ago

Was the ChampionMastery endpoint even part of 3.9?

taycaldwell commented 8 years ago

It was the main update for that version.

https://github.com/rithms/riot-api-java/releases

Linnun commented 8 years ago

Anyways, I guess this request is obsolete then.

briceambrosiak commented 8 years ago

Guys, the inconsistency is on method :

Class RiotAp => public net.rithms.riot.dto.Static.Champion getDataChampion(Region region, int id, String locale, String version, ChampData... champData) throws RiotApiException

It takes an int for the champion id, whereas it should be a long like ChampionMastery.getChampionId() is returning. I am in v3.9.0.

image

(BTW, why you closed the two Maven Central issues? I still can't find your library in it).

Linnun commented 8 years ago

ChampionMastery.getChampionId() returns an int.

getDataChampion() returns net.rithms.riot.dto.Static.Champion, and Champion.getId() also returns an int.

All Champion IDs currently have up to 3 digits, and even if Riot one day has champion IDs with 4 or even 5 digits, int will be more than enough to cover those IDs.

Please note that 3.9.0 still contains this inconsistency, but the current up-to-date branch doesn't. If you don't use the latest developer branch, please wait for the next release version to get this fix.

(About the Maven thing, I can't comment on that. Maven is rithms' thing)

taycaldwell commented 8 years ago

Reopened Maven issues for now.