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

TFT Implementation #80

Open
boraguler opened this issue Apr 14, 2020 · 14 comments
Open

TFT Implementation #80

boraguler opened this issue Apr 14, 2020 · 14 comments
Assignees
Labels
help-wanted We could use someone's help with this problem! implementation-outdated Current implementation of this library is outdated and needs an update.

Comments

@boraguler
Copy link

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

@dolejska-daniel dolejska-daniel self-assigned this Apr 15, 2020
@dolejska-daniel dolejska-daniel added the implementation-outdated Current implementation of this library is outdated and needs an update. label Apr 15, 2020
@dolejska-daniel
Copy link
Owner

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.

@boraguler
Copy link
Author

boraguler commented Apr 15, 2020

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?

@dolejska-daniel
Copy link
Owner

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 🥳.

@boraguler
Copy link
Author

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 :)

@dolejska-daniel dolejska-daniel added the help-wanted We could use someone's help with this problem! label Jun 11, 2023
@TheMY3
Copy link

TheMY3 commented Jun 13, 2023

@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.

@dolejska-daniel
Copy link
Owner

@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
Copy link
Owner

@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.

@TheMY3
Copy link

TheMY3 commented Jun 13, 2023

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

@TheMY3
Copy link

TheMY3 commented Jun 14, 2023

@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)

@dolejska-daniel
Copy link
Owner

@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.

@TheMY3
Copy link

TheMY3 commented Jun 15, 2023

  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
Copy link

TheMY3 commented Jun 24, 2023

Done - dolejska-daniel/riot-api-tft#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 🤔

@TheMY3
Copy link

TheMY3 commented Jul 10, 2023

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

@TheMY3
Copy link

TheMY3 commented Oct 13, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help-wanted We could use someone's help with this problem! implementation-outdated Current implementation of this library is outdated and needs an update.
Projects
None yet
Development

No branches or pull requests

3 participants