search

avivbiton / BlizzardApiReader

.Net Core library to handle Blizzard public APIs
MIT License
35 stars 36 forks source link

Convert all model properties to PascalCase #32

Closed linahanner closed 5 years ago

linahanner commented 5 years ago

The JSON objects returned by Blizzard does not adhere to the normal .NET & C# naming conventions. This has caused a scenario in which we are currently either deviating from the normal .NET & C# conventions in the models or annotate each property with a JsonProperty attribute causing potential confusion for developers using the library or tireless annotation work.

Examples:

One solution in order to be able to solve this particular problem is by introducing a custom ContractResolver which we can use for JSON deserialization from the content string of the response.

A ContractResolver which is able to...

By default the JsonConvert.Deserializer already turns camelCase into PascalCase.

linahanner commented 5 years ago

Did some digging, as mentioned in the previous comment the JsonConvert.DeserializeObject(json) by default deserializes camelCase into PascalCase.

I have yet to see a scenario in which Blizzard does not use camelCase - however the Hero class contains the property last_updated which brings me to believe the json data returns the field as snake_case. Could I have some clarity on this @avivbiton? I am not able to do the hero request since I don't have Diablo 3.

In the event that they use snake_case it is possible to use the following annotation (for that specific field): [JsonProperty(NamingStrategyType = typeof(SnakeCaseNamingStrategy))]

I will change the title of this issue to better reflect what we might want to do:

The reason I suggest we do this is to keep the naming consistent throughout the project, this way we don't have some models with Hero.last_updated other with Class.LastUpdated and yet a third one with Achievement.lastUpdated. The reason why I suggest PascalCase in particular is because it is the most prevalent naming standard within the .NET space.

avivbiton commented 5 years ago

Yes last_updated is snake case.
The models was something I've done at the very start of the project and didn't pay much attention to the naming and wanted to revisit them later on but forgot about them.
I also wanted to test if the variable names matter, so it's like you said If I can just make every property PascalCase and it will still work.

I will make it clear in the contribute section that all models names should be PascalCase