Change static strings to enums

RyadaProductions commented 4 years ago

Current situation: We have static structs/strings in the classes that are inside of the enums folder. This is not necessary and causes a confussing API. Because we can just enter a hardcoded string that contains a weird value as a method parameter.

Proposal: Change all enum classes to actual enum types, and use [Description("")] to still be able to get a string value that is not equal to the actual enum value. This will allow us to make the API safer and less confussing to use.

Caveats: We will have to upgrade to .NET Standard 1.3 as the lowest supported version due to the necessity of System.Reflection.TypeExtensions since that is the lowest supported framework version for that Nuget package. Everything at or above .NET Standard 2.0 won't have to add a Nuget reference.

MingweiSamuel commented 4 years ago

I agree that the current API is confusing, enums have been a big mess for forever. I originally chose to use raw strings for robustness; the Riot API changes a lot without warning, so if some string enum suddenly changed the user would be able to still use Camille without needing to wait for an update. (tangent - if csharp had enum MyEnum : string or some sort of type aliases then we could have the best of both worlds)

Now after maintaining Camille for several years it seems those robustness concerns aren't actually relevant (maybe because I've been maintaining it), and it also seems Riot has stepped up their doc game for constants, so it is probably worth switching to enums. It also seems that the vast majority of strings in Queue.cs don't show up there, so I'm going to assume they aren't used anymore, so it's probably best to just mark that class deprecated and leave it as-is.

So next, I don't love the reflection. I was thinking, given those new .json docs, we could probably just generate files like is currently done with Champion. We can also make proper lookup-by-string methods and json ser/de for that.

Now we should probably also have DTO classes use these enums in their fields. I guess that can be a TODO. I might want to make changes on riotapi-schema's end for that.

Let me know what you think. And thanks for the PRs!

RyadaProductions commented 4 years ago

After I made the PR I was thinking about seeing if there would be a way to automatically generate the Enums based on their API, luckily they have the new docs for constants. So we could most likely switch to generated Enums.

The reflection was just there to be able to get an arbitrary string without creating a map of Enum value -> string. The maps add a second point of failure as it is not directly related to the Enum itself which causes maintainability overhead. especially since I am not a fan of static maps The reflection is minor in terms of performance, and gives us

Generating the Enums based on the .json files sounds good to me, but I am not sure if we would want the reflection based approach or the map based approach. Both solutions have their problems and benefits.

The only problem I see with adding it to the DTO's is that the DTO specification doesn't list a constant on the docs at least. It just shows a string value as the type on the DTO. But we might be able to match the Enums with some clever enhancements of a simple string matching scheme.

Edit: Some .json links by them currently contain an incorrect json format, which will sadly prevent us from generating some automatically. For example the gameTypes.json

RyadaProductions commented 4 years ago

As an additional suggestion it might be cool to see if we can add the DataDragon API to the library as well once we are done with adding the Enums to the api.

MingweiSamuel commented 4 years ago

The maps add a second point of failure as it is not directly related to the Enum itself which causes maintainability overhead.

I agree in general but if its generated it shouldn't be a problem.

I am not sure if we would want the reflection based approach or the map based approach

I wasn't thinking a Dictionary. Initially was thinking hardcoded switch statements BUT since these enums will be tightly-packed we can just use an array static string[] as lookup indexing on the enum (with bounds checks ofc). Would that solve your objections to Dictionary? :P

Reflection is insignificant overhead but given that its generated I'd like the "fast" way better.

the DTO specification doesn't list a constant on the docs at least. It just shows a string value

Yeah, it would have to be something hardcoded into that project. There are already some hardcoded overrides to address some quirks: https://github.com/MingweiSamuel/riotapi-schema/tree/master/src/data

Edit: Some .json links by them currently contain an incorrect json format, which will sadly prevent us from generating some automatically.

Seems it also has some other errors: https://github.com/RiotGames/developer-relations/issues/170

So maybe it would be worth making a riotapi-constants projects that updates daily like riotapi-schema which could fix those errors for now and also work as a log of any changes. Though maybe thats overkill

MingweiSamuel commented 4 years ago

Oh and also made #16 for ddragon/cdragon

RyadaProductions commented 4 years ago

What we could also do instead of creating a backing storage of strings. Is use the Display attribute, and make the Enums have the same name as the value that would go into the actual api. So that we inside of the API only have to do .ToString() and then we would have the proper value. While if people want to have a friendly name they could get the friendly name from the Display attribute.

If we are going to generate them anyway I don't see why we shouldn't make them mirror the actual API value, instead of a Friendly name. This would also make it more consistent if people look at the Riot API to see what they need and use values inside of their code to interact with it. Although I am not entirely sure if this would be acceptable from a fast perspective.

MingweiSamuel commented 4 years ago

Ok here's the plan:

seasons - `int` enum

    {
        "id": 0,
        "season": "PRESEASON 3"
    }

becomes

        PRESEASON_3 = 0,

queues - `int` enum

    {
        "queueId": 2,
        "map": "Summoner's Rift",
        "description": "5v5 Blind Pick games",
        "notes": "Deprecated in patch 7.19 in favor of queueId 430"
    }

becomes

        /// <summary>
        /// 5v5 Blind Pick games
        /// <para />Deprecated in patch 7.19 in favor of queueId 430
        /// </summary>
        [Description("5v5 Blind Pick games")] // unnecessary?
        [Map(1)] // custom attribute?
        5V5_BLIND_PICK_GAMES = 2

maps - `int` enum

{
        "mapId": 1,
        "mapName": "Summoner's Rift",
        "notes": "Original Summer variant"
    }

becomes

        /// <summary>
        /// Summoner's Rift
        /// <para /> Original Summer variant
        /// </summary>
        [Description("Summoner's Rift")] // Useful?
        SUMMONERS_RIFT = 1

gameModes - `string` enum

    {
        "gameMode": "CLASSIC",
        "description": "Classic Summoner's Rift and Twisted Treeline games"
    }

becomes

        /// <summary>
        /// Classic Summoner's Rift and Twisted Treeline games
        /// </summary>
        [Description("Classic Summoner's Rift and Twisted Treeline games")] // Useful?
        CLASSIC,

parse using the name ("CLASSIC")

gameTypes - `string` enum

    {
        "gametype": "CUSTOM_GAME",
        "description": "Custom games"
    }

becomes

        /// <summary>
        /// Custom games
        /// </summary>
        [Description("Custom games")] // Useful?
        CUSTOM_GAME,

parse using the name

Can get the descriptions using reflection, assuming those values are actually problematically useful to get

Then for the other enums

GameResult can be Win, Lose, and just parse using the name
Locale can be just en_US and parse using the name
Position can be parsed using the name -- also check if this is still used since this originated from positional ranks which are gone
static class Queue stays the same but gets marked deprecated
Tier can be CHALLENGER, parse using the name
TeamId should definitely just be normal enums 100, 200

And finally region... I guess just naming them BR1, EUN1, EUW1 and using ToString() is fine. (or array lookup

And of course this would all be version 3.0.0

RyadaProductions commented 4 years ago

Do we want to make them generated instantly of do we first want to make a commit with changing them manually and then convert them to be generated later on?

MingweiSamuel commented 4 years ago

Can just make those 5 ones from the docs generated, no point in doing the work twice, but the other ones can just be manual.

RyadaProductions commented 4 years ago

This is fixed with pr #15

RyadaProductions commented 4 years ago

Appologies for marking this as fixed, this was not fixed to be completely honest. I just noticed i forgot to do a bunch of things to finish this. The newly generated Queues enum is currently invalid and needs to be changed to be able to work as part of the API. Is one of the things that are currently broken. It only reflects the outbound QueueId value, instead of the inbound QueueName that the League API expects. I would have to create a second enum to achieve this as there were initially already name collisions in the Queues enum

MingweiSamuel commented 4 years ago

Yeah I was wondering about that, seems pretty tricky

RyadaProductions commented 4 years ago

I have been working on a branch that contains the Enum API with generation.

RyadaProductions commented 4 years ago

The only Issue I currently have with that branch is that the Enum needs to be casted to the underlying int value on some API's. And that it has a gigantic switch which is basically a name mapping, I want to improve it but I'm not really great with JS

MingweiSamuel commented 4 years ago

Ah yes, I should add those mappings on https://github.com/MingweiSamuel/riotapi-schema 's end

RyadaProductions commented 4 years ago

Okay my branch is currently in a working state. With the Enums added to the API. If you want I can create a PR with those changes. And then after you added the mappings to the schema we can update it again to generate it based on that instead of having the gigantic mapping in the dotUtils.js

MingweiSamuel commented 4 years ago

Ok, I've updated the schema to contain an x-enum field which is one of the following values: ["queueType","queueId","champion","division","tier","season","gameType","gameMode","map","locale","team"].

You should be able to just check prop['x-enum'] and get rid of checkApiNameForEnum(name);, and also recursive [] case should be fine as well.

As another note, I am planning on figuring out which fields are actually nullable, so we can have proper ? https://github.com/MingweiSamuel/riotapi-schema/issues/7

RyadaProductions commented 4 years ago

There is a small issue with the new mapping that was added, the enum mappings aren't done in PascalCase which makes it so I have to capitalize it (there is already a method for it but i think its better to change it so that it reflects the actual enum name) as well as the fact that the mappings have a singular name, instead of a plural like i am using. Also some of the mappings don't follow the current names of the enums at all. I am not sure if this is intentional and you want to change the enum names.

MingweiSamuel commented 4 years ago

Oo I didn't think about that at all.

Lets change the enums to be singular since that is standard & looks better in fn args

I will change queueId to queue since it's listed in queues.json and the "Id" doesn't add any meaning.

Then team, change the enum name TeamId -> Team for the same reason as above, unless you have a strong opinion about it.

Then queueType I'm the most conflicted about. I want to keep it the same on the schema since it's meant to be a 1:1 representation of the docs. But RankedQueue is more descriptive. I'm fine with either, (would need an extra if in the .js for RankedQueue)

Sorry for all these slow small tedious changes. Seems unfortunately our timezones are misaligned.

I also would like to update the Queue enum to include the deprecated queue ids since some can still show up in old match histories, probably just in the form:

SUMMONERS_RIFT_5V5_RANKED_SOLO_GAMES_DEPRECATED = 4
SUMMONERS_RIFT_5V5_RANKED_SOLO_GAMES = 420

If you have any thoughts or objections to that, otherwise I'll add that myself

RyadaProductions commented 4 years ago

I have made all the required changes. But the .spec.json currently only has 1 problem. And that is the parameters of the GetMatchList endpoint. The Queue is listed as id instead of the enum type so the automatic generation fails. I could manually fix it but it would be better to fix it in the .spec.json imo.

MingweiSamuel commented 4 years ago

Oops, my bad, definitely a bug. Should be fixed now https://github.com/MingweiSamuel/riotapi-schema/commit/0941e4d20828cbea96ec3fae80cd9d030856da1b

RyadaProductions commented 4 years ago

The Queue enum is quite weird now, it looks like this:

[Display(Name = "Summoner's Rift", Description = "Co-op vs AI Beginner Bot games")]
SUMMONERS_RIFT_CO_OP_VS_AI_BEGINNER_BOT_GAMES_DEPRECATED = 32,
[Display(Name = "Summoner's Rift", Description = "Co-op vs AI Intermediate Bot games")]
SUMMONERS_RIFT_CO_OP_VS_AI_INTERMEDIATE_BOT_GAMES_DEPRECATED = 33,
[Display(Name = "Twisted Treeline", Description = "3v3 Ranked Team games")]
TWISTED_TREELINE_3V3_RANKED_TEAM_GAMES_DEPRECATED = 41,

I am not quite sure if this would be the preferred way, but it would be nice if we could somehow clean up the clutter. Maybe create a mapping so that the map names will be the shorthand versions? Or do you have other suggestions? Leaving it as is would be fine as well though.

MingweiSamuel commented 4 years ago

Ah I wasn’t expecting you to do that too, thanks! Yeah that is a good idea, SR and TT are commonly used.

Up to you though, you’re doing a lot :)

MingweiSamuel commented 4 years ago

Also as a tracking issue, summerSpellId is probably another good candidate for enumification Though that might be a ddragon listing?

Edit: I guess the question is at this point, how much should be included from ddragon as enums in code (such as champions are), and how much should be put into #16 ddragon support. Some cases:

Champions (already included)
Items (there are a lot - too many for an enum probably)
Summoner spells
Profile icons (probably not that useful to include)
Perks (runes reforged)

Remaining info for ddragon would be detailed champion info, detailed item info, and image data.

RyadaProductions commented 4 years ago

The QueueType enum still needs to exist unfortunately unless we find a way to generate that based on the spec as well. That specific enum needs to exist for the .ToString() on specific end points, as it isn't provided by any of the data endpoints of DDragon/CDragon. I was planning to ask, would it be okay if i also revamped the generation of the Champion enum? So it would be in line with all the other generated Enums. Also i think it is a good idea to remove the Obsolete enums to lessen the clutter and not run into name collisions, in the future.

Regarding DDragon. I think it would be best if we include as much as we can without making it too confussing, as it would help streamline the library usage in the future. only partially including data is kinda confussing imo

MingweiSamuel commented 4 years ago

Well if you want to update champion go for it :p

MingweiSamuel commented 4 years ago

What's your summoner name & region? I can add you to the app group so you can use the same encrypted keys

RyadaProductions commented 4 years ago

My summoner name and reguion is: Ryada - EUW I will update the Champion enum as well and then make the PR for at least the majority of the changes we spoke of.

RyadaProductions commented 4 years ago

After testing the enum api in an application it is working perfectly, although a minor issue is that the Enum names from the champions don't match the DDragon names of the champions. Changing KAI_SA to KAISA would solve this but I forgot if we did this on purpose, or if it was leftover from the previous API

MingweiSamuel commented 4 years ago

I don’t think the riotapi uses the string version, so it should be fine to change

MingweiSamuel commented 4 years ago

I believe this issue was resolved 25f16f6ada7ef69798ae6b51686317461d64d71a

MingweiSamuel / Camille