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

Add MATCH-V5 Support #685

Merged
merged 8 commits into from
Jun 18, 2021
Merged

Add MATCH-V5 Support #685

merged 8 commits into from
Jun 18, 2021

Conversation

xXLAOKOONXx
Copy link
Contributor

This Pull request adds support for MATCH-V5:

  • Matches by Puuid
  • Match by MatchId
  • Timeline by MatchId

Additionally the functionalities of the Tournament API should be unchanged. (But the Namespace for the returned Models changed)

This Pull request does not include any changes towards testing. I just have a 'worked on my machine'.

Furthermore there could be some improvement done in regards of some of the values. I did not create any new enum type. I might used a few too often times 'long' instead of 'int'. Documentation from Riot is not there yet, so there is some guessing involved.

Ticket #684

@xXLAOKOONXx
Copy link
Contributor Author

Information from Riot:

match-v4 is being deprecated on June 14th, 2021. Please use match-v5 as a replacement. match-v5 is available on Development API keys as of April 14th, production access will follow in the coming weeks/month.

The match and timeline responses are not 100% locked. There may be minor field changes, which will be documented in the method details. We'll make our best effort to extend the deprecation window if any changes happen after match-v5 becomes available for production API keys.

https://developer.riotgames.com/apis#match-v5

@xXLAOKOONXx
Copy link
Contributor Author

I did not validate if tournament api is still working as I can not test this!

Copy link
Owner

@BenFradet BenFradet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, that looks good to me 👍

That PR makes me wonder if we should ditch tournament support, wdyt?

@BenFradet BenFradet requested a review from JanOuborny May 21, 2021 10:17
@xXLAOKOONXx
Copy link
Contributor Author

On twitter riot gave some information on theire plans: https://twitter.com/RiotGamesDevRel/status/1395091864408129536

"We're actively working on solutions to ensure continued support for tournament codes [...]"
so there might be an update in that regards in near future.

I do not understand the current Implementation of the tournament match portion. I found a url snippet that is used but I can not relate it to any current API provided:

private const string MatchRootUrl = "/api/lol/{0}/v2.2/match";

So it might already be the case that there is no support for getting tournament match information (getting codes etc looked fine).

My proposal is to keep the tournament api as is and once Riot adds a new requesturl for tournament matches this requesturl should be implemented in the regarding endpoint (most likely MatchEndpoint) and not costum crafted into the TournamentRiotApi class.

Another option would be to dump all tournament support. This would clearify the identity of RiotSharp from 'reflecting riot apis' towards 'making data from riot apis available', which is what most framework users are here for, i guess. For this decision I do not have an insight in whether the tournament part is actually in use by someone.

@BenFradet
Copy link
Owner

My proposal is to keep the tournament api as is and once Riot adds a new requesturl for tournament matches this requesturl should be implemented in the regarding endpoint (most likely MatchEndpoint) and not costum crafted into the TournamentRiotApi class.

I think that makes the most sense for now, thank you for the investigation 👍

@selmaohneh
Copy link
Contributor

any news here? can this be merged?

@BenFradet
Copy link
Owner

I was waiting on @JanOuborny 's review but we can merge now

@BenFradet BenFradet merged commit 3891550 into BenFradet:develop Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants