search

avivbiton / BlizzardApiReader

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

Suggested changes to API migration #39

Closed linahanner closed 5 years ago

linahanner commented 5 years ago

Suggested changes for pull request #38 which migrates the API from the old Developer portal to the new Developer portal requiring OAuth.

When using the WorldOfWarcratAPI or DiabloApi I was unable to send a request token, instead implemented automatic retrieval of token if the token either...

  1. Does not exist
  2. Has expired
avivbiton commented 5 years ago

I forgot to mention, I changed ApiConfiguration a bit, I removed the constructors since I felt they were unnecessary since we can just use FluentAPI to construct the configuration.

Also removed some of the comments because they felt redundant and unnecessary (Like a comment the explains the the method SetRegion, sets the region)

The changes looks good, I will merge them but what do you think about dropping the constructors ?

linahanner commented 5 years ago

Yes, absolutely! Hadn't heard of FluentAPI before, actually, I'll make some adjustments real quick =)

linahanner commented 5 years ago

Have removed the additional constructors.

Since Enum values cannot be null, Region and Locale will automatically receive default values. I wanted to make sure the default Region and Locale of any newly created ApiConfiguration always match, if neither the Region nor the Locale has been touched. To enforce this I made sure the Locale is the default locale of the Region.