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

Enum api #22

Merged
merged 5 commits into from Nov 6, 2019

Conversation

@RyadaProductions
Copy link
Contributor

RyadaProductions commented Nov 1, 2019

This should finish up the majority of all the enumization throughout the system. In addition to the usage of [Display()] so that other developers can still get a UI friendly name.

@RyadaProductions RyadaProductions force-pushed the RyadaProductions:Enum-Api branch from 4f6a27e to 001dbf5 Nov 1, 2019
@RyadaProductions

This comment has been minimized.

Copy link
Contributor Author

RyadaProductions commented Nov 4, 2019

@MingweiSamuel would you be able to take a look at this PR?
I opted to leave the enums as is, and maybe change them later on. As i would like to have some real life tests going soon on the Enum api.

Copy link
Owner

MingweiSamuel left a comment

Sorry, lost track of this one.

I'm not sure about the [System.Text.Json.Serialization.JsonConverter(typeof(System.Text.Json.Serialization.JsonStringEnumConverter))] attributes since most of the enums are represented using their number. How do they work?

Edit it seems to work with and without it on core 3.1?

Camille/gen/dotUtils.js Outdated Show resolved Hide resolved
Camille/gen/Enums/Champion.cs.dt Outdated Show resolved Hide resolved
public enum Tier
{
CHALLENGER,
GRANDMASTER,
MASTER,
DIAMOND,
PLATINUM,
GOLD,
SILVER,
BRONZE,
IRON,

/// <summary>In most endpoints, tier will not be provided if related summoner is unranked.</summary>
UNRANKED,
}
Comment on lines +9 to +23

This comment has been minimized.

Copy link
@MingweiSamuel

MingweiSamuel Nov 5, 2019

Owner

This is from the previous commits, but in the current version 0 or missing values will get parsed as Challenger. API seems to have stopped returning highestAchievedSeasonTier so that is one place where that is noticeable.

This will be fixed with proper DTO field nullability which I've already annotated riotapi-schema with. However we should probably start the non-numeric enums from 1 to catch any future occurrences.

This comment has been minimized.

Copy link
@RyadaProductions

RyadaProductions Nov 5, 2019

Author Contributor

I am not sure how the returning values work, wouldn't it in this case make sense to put UNRANKED as the 0 in the enum?

This comment has been minimized.

Copy link
@MingweiSamuel

MingweiSamuel Nov 5, 2019

Owner

UNRANKED wouldn't be the same as no value, it seems highestAchievedSeasonTier is straight-up removed RiotGames/developer-relations#94

I'm not worried about it too much, just something I took note of

@RyadaProductions

This comment has been minimized.

Copy link
Contributor Author

RyadaProductions commented Nov 5, 2019

The [System.Text.Json.Serialization.JsonConverter(typeof(System.Text.Json.Serialization.JsonStringEnumConverter))] is necessary in 3.0 because somehow when deserializing the objects it can't properly convert a string to enum value. This helps getting around it, and makes us able to use it in 3.0, aside from the weird bug with deserializing

@MingweiSamuel MingweiSamuel merged commit 9db655c into MingweiSamuel:master Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.