tamland / python-tidal

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

Spec of Session should be fully exposed on class #192

Open 2e0byo opened 1 year ago

2e0byo commented 1 year ago

Session has some attributes like genre and request which should be exposed on the class. Otherwise if a Session is mocked (with Mock(spec=Session)) access to these attributes is forbidden. (See the mock docs for a description of this problem and why speccing is important.)

We worked around this on mopidy-tidal with a nasty hack. The test suite rewrite (in progress) subclasses and links to this bug.

All these attribute should be set on the class. Additionally it'd be nice now we have type hints to remove the old 'default None' which causes so many spurious errors and just set the type, like so:

# Bad old way, pre typehints
class Thing:
    x = None
    y = None
    def __init__(self, x, y):
        self.x = x
        self.y = y

 # Elegant new way       
class Thing:
    x: str
    y: int
    def __init__(self, x: str, y: int):
        self.x = x
        self.y = y

The typehint work by @arusahni had some pretty heroic workarounds for some of this kind of pattern. A bit of refactoring will make things a lot easier.

I'll implement this at some point if nobody wants to do it first, but it probably won't be for at least a month.

arusahni commented 1 year ago

Some of the None/Optional occurrences are also necessary for the pattern we use for parsing. e.g., Session.parse_album first instantiates an empty Album (i.e., no album ID or other metadata) and then calls the parse() method with the API response object to hydrate the instance. It then returns a copy of itself, which feels like an unnecessary allocation.

I think migrating from this pattern to classmethods (e.g., from_response(cls, api_object)) would allow us to drop the None types that have crept in.

2e0byo commented 1 year ago

Yes, that should go on the list too. Currently the hydration pattern plays havok with downstream libraries, which either have to assume they got a properly hydrated instance, or have loads of redundant checks. I think the PR for this issue will probably be quite speculative, and we should discuss it carefully.