Add typing to the Playlist and Page classes #191
Conversation
|
Thanks, I will try to test this asap, as I just added a few features to mopidy-tidal that requires Page objects. |
|
Hmm. Thanks for this. I think you've exposed a real weakness in the class hierarchy here. I've not had a massive amount of thought about it, but instinctively I'd prefer making the spec of all the response types the same (even with a no-op .items()), preferably by making them inherit and implement an ABC. For the time being @arusahni could you do it the other way, i.e. change the class? As a more general point I'd like to have a shot at thinning out the codebase a bit. The api as it stands is quite fragmented, something which lead to some curious mocking tricks when I first wrote the test suite for mopidy-tidal. Getting typing (and testing) working is a good way to avoid nasty surprises, but I'm personally up for changing our api (with careful consideration) to get rid of some of these inconsistencies. As a step in that direction I think it would be good if PRs started proposing api improvements, i.e. don't feel afraid @arusahni to suggest changes. At some point we need to rewrite the config class at least a bit. I don't have massive amounts of time before Christmas, but I might manage to get some work done on things in december. If anyone's browsing by and want to have a shot at refactoring, go for it. |
| self.request = session.request | ||
| self.categories = None | ||
| self.title = title | ||
| self.page_category = PageCategory(session) | ||
|
|
||
| def __iter__(self): | ||
| def __iter__(self) -> "Page": |
There was a problem hiding this comment.
(aside: when we move to 3.11 these should become typing.Self to handle children.)
arusahni
force-pushed
the
master
branch
2 times, most recently
from
7ebead8
to
1505375
Compare
arusahni
changed the title
|
@2e0byo thanks for the review! I sprinkled a bunch of
I've updated this PR to include typings for |
|
@arusahni I did some quick tests in Mopidy-Tidal. It looks like everything is working as expected when browsing various pages so at least it looks like these changes didn't break anything. |
|
Thanks @arusahni yet again for the work. I'll run the test suite at some point and then merge this in. |
|
Any more comments on this PR @2e0byo? Otherwise lets merge it in and prepare for next tidalapi release. |
|
@tehkillerbee if the tests pass all is good. Would you check? Thanks for picking this up. |
| popularity = -1 | ||
| promoted_artists = None | ||
| last_item_added_at = None | ||
| popularity: Optional[int] = -1 |
There was a problem hiding this comment.
We should probably set this one to None to be consistent with L94
| @@ -83,7 +90,8 @@ def parse(self, json_obj): | |||
| self.created = dateutil.parser.isoparse(created) if created else None | |||
| public = json_obj.get("publicPlaylist") | |||
| self.public = None if public is None else bool(public) | |||
| self.popularity = json_obj.get("popularity") | |||
| popularity = json_obj.get("popularity") | |||
| self.popularity = int(popularity) if popularity else None | |||
There was a problem hiding this comment.
This breaks the tests, as we expect default value to be 0. However, your changes are more consistent so lets update the tests to expect None
|
LGTM, will fix remaining review suggestions in a separate PR |
This was the hairiest of the type conversions so far. It was complicated by the fact that the
Artistclass does not have an items collection. As a result, thePageiterator interface would end up raising anAttributeErrorif iteration was attempted. Rather than add a no-opitemsmethod to the theArtisttype, I opted to shim it at thePagelevel to keep the API clean.Please nitpick this! While I use the API, I've never used this module.
Advances #137