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

Implement api/tournament endpoints #50

Merged
merged 20 commits into from
Nov 2, 2023

Conversation

Anupya
Copy link
Contributor

@Anupya Anupya commented Oct 20, 2023

Description

  1. Add all the remaining tournament endpoints:
  • get_team_standings = /api/tournament/{id}/teams (there is no /api/swiss equivalent)
  • update_team_battle = /api/tournament/team-battle/{id} (there is no /api/swiss equivalent)
  • join_arena = /api/tournament/{id}/join
  • terminate_arena = /api/tournament/{id}/terminate
  • withdraw_arena = /api/tournament/{id}/withdraw
  1. Add missing docstring for get_tournament params
  2. Fix typos and make id uppercase

After merge

Please update #6 and check off all the five api/tournament/... endpoints in the issue description.

Also there are several endpoints in the issue description that have already been implemented but not been checked off. I think there's only 6-7 endpoints left in that list and everything else can be checked off.

@Anupya Anupya marked this pull request as ready for review October 20, 2023 20:50
@kraktus
Copy link
Member

kraktus commented Oct 21, 2023

Hey, thanks for the PR, since tournament is already the name of the client, you can remove it from the method name.

Copy link
Member

@benediktwerner benediktwerner left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the typos!

Re: return type annotations, ideally, we'd like to have accurate return types using TypedDicts for new endpoints. See #38 for an example. Although I believe for tournaments, the return types are in some cases very complex and not 100% correct on the wiki, in which case it might be ok to leave them out. But best to check and we'd at least still want something like Dict[str, Any].

Also, for endpoints like withdraw that only return "ok", it's better to not return anything (but they should still get a -> None annotation). The result in this case will always be "ok" since it will throw if it errors, so returning something gives the incorrect impression that the return value has real meaning and should be checked and can also obscure the fact that errors will throw.

berserk/clients/tournaments.py Outdated Show resolved Hide resolved
berserk/clients/tournaments.py Show resolved Hide resolved
berserk/clients/tournaments.py Outdated Show resolved Hide resolved
berserk/clients/tournaments.py Outdated Show resolved Hide resolved
berserk/clients/tournaments.py Outdated Show resolved Hide resolved
berserk/clients/tournaments.py Outdated Show resolved Hide resolved
berserk/clients/tournaments.py Outdated Show resolved Hide resolved
berserk/clients/tournaments.py Outdated Show resolved Hide resolved
berserk/clients/tournaments.py Outdated Show resolved Hide resolved
berserk/clients/tournaments.py Outdated Show resolved Hide resolved
@Anupya Anupya force-pushed the Anupya/add-tournament-endpoints branch 3 times, most recently from caffb3b to efef654 Compare October 30, 2023 14:25
}
self._r.post(path=path, params=params, converter=models.Tournament.convert)

def get_team_standings(self, tournament_id: str) -> Dict[str, Any]:
Copy link
Member

Choose a reason for hiding this comment

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

the response is simple enough to be typed: https://lichess.org/api#tag/Arena-tournaments/operation/resultsByTournament please also include a test for it

@Anupya Anupya force-pushed the Anupya/add-tournament-endpoints branch from efef654 to 986effa Compare November 1, 2023 19:17
@Anupya
Copy link
Contributor Author

Anupya commented Nov 2, 2023

Update - I am having trouble making that test pass due to my Python version being wrong and my local being weird. I'm gonna be busy at work again so may not have time to get to it. Please fix it if you can 🙏 but the other stuff should work.

@kraktus kraktus force-pushed the Anupya/add-tournament-endpoints branch from d5cf59c to 5dcba7c Compare November 2, 2023 21:13
@kraktus kraktus merged commit f4cafbe into lichess-org:master Nov 2, 2023
21 checks passed
@kraktus
Copy link
Member

kraktus commented Nov 2, 2023

Thanks again for the numerous PRs! Quite a significant leap in coverage

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