client.broadcasts: add get_top (types, tests, docs) #128
Conversation
tests/clients/test_broadcasts.py
Outdated
|
|
||
|
|
||
| @skip_if_older_3_dot_10 | ||
| def test_get_top_valid(requests_mock): |
There was a problem hiding this comment.
This test does not test anything. There’s no logic in the code, so no need to test that. We actually want to check these server’s response, so it needs to be a VCR test, see CONTRIBUTING.
There was a problem hiding this comment.
I removed the mock test and added a VCR test as documented in CONTRIBUTING.
berserk/types/broadcast.py
Outdated
| class BroadcastTopResponse(TypedDict): | ||
| """Minimal TypedDict for /api/broadcast/top response.""" | ||
|
|
||
| # List of active broadcasts |
There was a problem hiding this comment.
Needs to be more precise to be useful
There was a problem hiding this comment.
I added the Type as shows in the lichess API/Docs/Specs/Schema
|
Thanks for the swift review. I didn't knew to avoid instance validation at runtime if using typechecking. I will work on these change to make sure the code is on par with the standard. I will set the PR back to draft while I work on this issue. |
- Converted the test function into TestBroadcasts class. - Added pytest.vcr - Removed mock test
|
@kraktus Thanks again for the review. I have adressed the feedback:
I have also taken the liberty of renaming the Type Object The PR should now be ready for another look. |
This PR is a small part of #6 and implements the broadcasts.get_top endpoint
Summary:
Validation:
Checklist when adding a new endpoint
README.mdclient.users.get_user(), Correct:client.users.get()berserk/types/, exampleCHANGELOG.mdin theTo be releasedsection (to be created if necessary)