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

Spec of Session should be fully exposed on class #192

Open
2e0byo opened this issue Sep 3, 2023 · 2 comments
Open

Spec of Session should be fully exposed on class #192

2e0byo opened this issue Sep 3, 2023 · 2 comments

Comments

@2e0byo
Copy link
Collaborator

2e0byo commented Sep 3, 2023

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
Copy link
Contributor

arusahni commented Sep 8, 2023

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
Copy link
Collaborator Author

2e0byo commented Sep 9, 2023

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.

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

No branches or pull requests

2 participants