MingweiSamuel / Camille

C# Riot API Library. Thread safe, automatic retries, autogenerated nightly releases.
Other
102 stars 8 forks source link

Add custom domain support #103

Closed zbee closed 7 months ago

zbee commented 8 months ago

(Maintainer's note: moved and merged via #108)

I added support for custom domains in ApiConfig, with the region appended as a query parameter, and provided an example in the ReadMe.

All behavior is the same if you don't pass a custom domain, but otherwise it forms the called URLs with your custom domain + the endpoint + the region as a query parameter (with consideration to the few endpoints that have parameters already). This fits the recommended proxy format.

Finally, I also updated the ReadMe's example code to use the the correct ApiConfig and GetChampionMasteries.

(apologies for the ReadMe entire change, it matched line-endings on commit)

zbee commented 8 months ago

@MingweiSamuel Just wanted to try to bring this up your queue again. I have now tested it with the Account and 2 Match endpoints - one of which uses query parameters.

zbee commented 8 months ago

Thank you so much for reviewing this! I will look to get these changes implemented tonight.

I'll try to find other proxy scripts and see if I can support how they need the region passed, good suggestion.

MingweiSamuel commented 8 months ago

Yeah in Riven the url is configured with a format param {}:

https://github.com/MingweiSamuel/Riven/blob/8d810ccca86b29c86dd4eb4891bc4060faf8c4d3/riven/src/config.rs#L26-L29

Which works for subdomain and path prefixes, but not for query params

    /// `"https://{}.api.riotgames.com"`
    ///
    /// Default base URL, including `{}` placeholder for region platform.
    pub const DEFAULT_BASE_URL: &'static str = "https://{}.api.riotgames.com";
zbee commented 8 months ago

I have updated this, closing all but one of your change requests above.

I implemented the below suggestion, and an enum to choose how the region should be applied to the API call, to make it easier to support the region as a path parameter in the future, but I only directly added support for the region to be a header (and the existing query parameter and subdomain, of course).

Yeah in Riven the url is configured with a format param {}:

Thank you again for your review, and I welcome any further change requests of course!

zbee commented 7 months ago

@MingweiSamuel Just wanted to bump this up again, I implemented your and AoshiW's change requests barring one.

MingweiSamuel commented 7 months ago

Apologies for the slow response time, just missed the notifications

zbee commented 7 months ago

No worries at all about any slowness, I very much appreciate the time you've taken on this. I have completed your requested changes now, with one concern noted above, which doesn't seem to me to be something I am able to address further myself.

MingweiSamuel commented 7 months ago

Commits squashed and moved to #108