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

Added Data Dragon for TFT #245

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

geomimo
Copy link

@geomimo geomimo commented Aug 31, 2024

Added Data Dragon Endpoint for TFT assets. Passed tox tests on python 3.10.

P.S. first ever contribution, please let me know if I need to do any other test or something.

Copy link
Owner

@pseudonym117 pseudonym117 left a comment

Choose a reason for hiding this comment

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

In addition to other review comments, should add integration tests for these changes. See league ddragon integration tests for an example.

Overall, everything looks good, just small changes needed to merge.

@@ -1,3 +1,4 @@
from .LeagueApi import LeagueApi
from .MatchApi import MatchApi
from .SummonerApi import SummonerApi
from .DataDragonApi import DataDragonApi
Copy link
Owner

Choose a reason for hiding this comment

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

nit: add newline at end of file

tacticians = DDragonVersionLocaleEndpoint("/tft-tactician.json")
traits = DDragonVersionLocaleEndpoint("/tft-trait.json")
versions = DataDragonEndpoint("/realms/{region}.json")
versions_all = DataDragonEndpoint("/api/versions.json")
Copy link
Owner

Choose a reason for hiding this comment

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

nit: add newline at end of file

@@ -0,0 +1,14 @@
from ...league_of_legends.urls.DataDragonUrls import DataDragonEndpoint, DDragonVersionLocaleEndpoint
Copy link
Owner

Choose a reason for hiding this comment

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

dont import things from league APIs in TFT; copy+pasting classes is preferable here so that the games are not mixed up in any way


from riotwatcher._apis.team_fight_tactics import DataDragonApi

@pytest.mark.lol
Copy link
Owner

Choose a reason for hiding this comment

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

this should be @pytest.mark.tft

Comment on lines +11 to +25
mock_base_api = MagicMock()
expected_return = object()
mock_base_api.raw_request_static.return_value = expected_return

static_data = DataDragonApi(mock_base_api)

version = "234"
locale = "sdfasdf"

ret = static_data.arenas(version, locale)

mock_base_api.raw_request_static.assert_called_once_with(
f"https://ddragon.leagueoflegends.com/cdn/{version}/data/{locale}/tft-arena.json",
{},
)
Copy link
Owner

Choose a reason for hiding this comment

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

optional because legacy issues:

there is a more elegant way to not repeat this code in each function using a text fixture instead. See league matchv5 unit tests for an example.

@pseudonym117
Copy link
Owner

FYI CI failures appear to be either my fault or the codecov actions fault. I will look into that

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.

2 participants