riot-api icon indicating copy to clipboard operation
riot-api copied to clipboard

TFT Implementation

Open boraguler opened this issue 4 years ago • 14 comments

"question" Any ETA on TFT side @dolejska-daniel ?

boraguler avatar Apr 14 '20 23:04 boraguler

Hello @boraguler, I'm pleased to see you are still around!

The implementation of APIs for other games than League of Legends comes with necessary changes to library structure... I've been thinking for a while how to tackle the issue and I think I'll have to split this library to separate modules for each game.

I'll try to have something to show by the end of the month, but I cannot promise anything, since this will require decent amount of work.

dolejska-daniel avatar Apr 15 '20 08:04 dolejska-daniel

Hi m8, yeah I'm always around since my platform uses your awesome wrapper <3

Nowadays I'll be adding TFT to the platform with kinda all features. So I thought to go either write my own simplest workaround till you add necessary endpoints, or try to make them within this wrapper. Tho I'm not that much expert as you, but somehow I may manage creating necessary endpoints / models / dtos etc.

I tried to examine your new additions for TFT yesterday, and yes I've come to the same conclusion as you that TFT shall be a separate module. Also a side note, I've got an internal rumour that on June-July, new endpoints will be made for TFT, also Valorant is on the way. Guess need to separate them all.

How do you want me to proceed, by just cheking-out and trying to modify-pull to this repo? Or shall I fork and let you join on the new one?

boraguler avatar Apr 15 '20 11:04 boraguler

If you're up for the task feel free to implement missing endpoints for TFT in LeagueAPI.php, it honestly should be just copy-pasta of methods already implemented for TFT League resource with result objects changed based on what's on Riot's Dev portal.

I will have to separate these changes into their own module later, but it will surely help me if it has already been implemented! Feel free to send PRs to this repo 🥳.

dolejska-daniel avatar Apr 15 '20 11:04 dolejska-daniel

Aight then, may take few days to find some focused time due to current workload. Soz for my bad code composition, let me tell you that in advance :)

boraguler avatar Apr 15 '20 20:04 boraguler

@dolejska-daniel any ideas how to do this for now? I want to create repo with TFT support. I think it is looks like https://github.com/dolejska-daniel/riot-api-league but I do not understand some code for now, for example how to count resource values and what is it - https://github.com/dolejska-daniel/riot-api-league/blob/e7a6217b70ee93fbeedb68afea80b4e1a27cd07f/src/LeagueAPI/LeagueAPI.php#L157. But I think I can skip it for now and create DTO related to docs and all should works fine.

One more question) do you generate this code or write by yourself? I see a lot of docblocks and it needs huge time for create and support all of this.

TheMY3 avatar Jun 13 '23 05:06 TheMY3

@TheMY3 The resources are essential for internal per-resource caching. The identifiers are from https://developer.riotgames.com/apis#tft-league-v1 (the identifier is hidden in the HTML here, for example, resource_1484).

You are indeed correct, it will look the same as the LeagueAPI, but I'd like to separate it into a repository of its own (as shown here). The repository can be found here: https://github.com/dolejska-daniel/riot-api-tft.

If you wish to contribute, please create an issue (link/mention this issue) and corresponding PR -- I am more than happy to contribute to it as well with some pre-generated API objects (like in the case of League API: https://github.com/dolejska-daniel/riot-api-league/tree/master/src/LeagueAPI/Objects). I already have those for TFT endpoints.

dolejska-daniel avatar Jun 13 '23 06:06 dolejska-daniel

@TheMY3, by the way, there is a tool (https://github.com/dolejska-daniel/riot-api-docs_parser) to generate DTO classes from the docs automatically -- that is what I use to create them easily.

dolejska-daniel avatar Jun 13 '23 06:06 dolejska-daniel

@dolejska-daniel this is exactly what I was looking for, thanks

TheMY3 avatar Jun 13 '23 09:06 TheMY3

@dolejska-daniel have few questions)

  1. How about code style? Need to use same as in other repositories or can use more modern approach?
  2. How about copyrights? It is generated in Dto, but some files need to create manually. Need to add it to all files or can remove it?

There were more questions, but for now I forgot about them, I'll be back as soon as I remember, if you don't mind)

TheMY3 avatar Jun 14 '23 19:06 TheMY3

@TheMY3 Glad you are still looking into this 😄

  1. DummyData are actual responses from the live API. I can generate these for you with my API key.
  2. Sure, definitely go with a more modern approach; however, I'm not sure what is so old about the code style used - can you tell me? 😄
  3. Please include the copyright notice in all files in the library. It is important to note that the software is provided without any guarantees.

For now, open the PR, and I can help you with some of the parts I know.

dolejska-daniel avatar Jun 15 '23 05:06 dolejska-daniel

  1. DummyData are actual responses from the live API. I can generate these for you with my API key.

I found it, by problem in some tests for me that I set RU region, but for some endpoints saved to file some of EU regions, when tests with live data - tests passed, when store data - some tests failed. Do not found solution for now and think better to find EU profile and store data for it.

Upd. Nvm, found solution)

TheMY3 avatar Jun 15 '23 16:06 TheMY3

Done - https://github.com/dolejska-daniel/riot-api-tft/pull/2

If everything goes ok and it will be possible to reduce the dockblocks, then I think I can then get to valorant in future, maybe :thinking:

TheMY3 avatar Jun 24 '23 14:06 TheMY3

@dolejska-daniel maybe you miss notification, PR waiting for 3 weeks, I almost forgot about it :slightly_smiling_face:

TheMY3 avatar Jul 10 '23 15:07 TheMY3

@dolejska-daniel hello again, maybe I can fork this? If you have no time :(

TheMY3 avatar Oct 13 '23 09:10 TheMY3