search

tamland / python-tidal

Python API for TIDAL music streaming service
GNU Lesser General Public License v3.0
413 stars 110 forks source link

Add typing to the Playlist and Page classes #191

Closed arusahni closed 1 year ago

arusahni commented 1 year ago

This was the hairiest of the type conversions so far. It was complicated by the fact that the Artist class does not have an items collection. As a result, the Page iterator interface would end up raising an AttributeError if iteration was attempted. Rather than add a no-op items method to the the Artist type, I opted to shim it at the Page level to keep the API clean.

Please nitpick this! While I use the API, I've never used this module.

Advances #137

tehkillerbee commented 1 year ago

Thanks, I will try to test this asap, as I just added a few features to mopidy-tidal that requires Page objects.

2e0byo commented 1 year ago

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.

arusahni commented 1 year ago

@2e0byo thanks for the review! I sprinkled a bunch of reveal_types for every item in a chained call and found two core issues that broke mypy's inference (pyright was fine):

  1. Broken typings on Session
  2. Missing typings for the playlist module.

I've updated this PR to include typings for playlist, and fixed #1. There are now far fewer casts.

tehkillerbee commented 1 year ago

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

2e0byo commented 1 year ago

Thanks @arusahni yet again for the work. I'll run the test suite at some point and then merge this in.

tehkillerbee commented 1 year ago

Any more comments on this PR @2e0byo? Otherwise lets merge it in and prepare for next tidalapi release.

2e0byo commented 1 year ago

@tehkillerbee if the tests pass all is good. Would you check? Thanks for picking this up.

tehkillerbee commented 1 year ago

LGTM, will fix remaining review suggestions in a separate PR