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

Organize API with Mixin Classes #34

Merged
merged 1 commit into from
Jan 21, 2024
Merged

Conversation

Erotemic
Copy link
Contributor

@Erotemic Erotemic commented Dec 2, 2023

This builds on top of my previous PR.

The idea here is start organizing the API to make it easier to keep track of what API calls have a Python call, and which don't. Currently the change is minimal to demonstrate the idea.

Basically for each group of related functionalities the idea is to make a single "mixin" class that contains those methods. Then the API class is a nearly empty class that just inherits from all of the "mixin" classes, thus providing a backwards compatibility.

However, now we don't need comments to delineate grouping of functionality as it is implicit in the code.

As an example of what I'm trying to do, I've had a lot of success with this pattern in my CocoDataset class. I think this pattern will have value here as well.

The current group of mixins uses the same structure implied by the comments. We have:

  • InternalAPIMixin - for the protected internal methods
  • BiggerAPIMixin - which was just labeled as the "Bigger section of the Jellyfin api". Not sure if there is a better name for that.
  • GranularAPIMixin - same deal as above, I just named it based on the comment.
  • SyncPlayAPIMixin - This one makes a lot of sense, its functions related to sync play
  • API - this inherits from all of the above, which means this PR introduces no user-facing changes. It just makes it easier to organize and continue development.

I also added a doctest to API, which won't run, but illustrates how to create an instance of it. If the testing tools have something that can spin up a dummy server, then we could make this test actually run with xdoctest.

@Erotemic Erotemic mentioned this pull request Dec 3, 2023
@iwalton3
Copy link
Member

This idea seems reasonable enough to me and would improve readability without breaking the API. I say go for it.

@Erotemic
Copy link
Contributor Author

I rebased on master and dropped other commits not related to this PR. Should be good to review & merge.

@Erotemic
Copy link
Contributor Author

@iwalton3 If you have any time, merging this would make it easier to me to push up new API features. I've written endpoints for exteranl provider (e.g. IMDB) identification, collection creation, and metadata editing that will have a cleaner diff once this is merged.

@iwalton3 iwalton3 merged commit 0d71331 into jellyfin:master Jan 21, 2024
5 checks passed
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

Successfully merging this pull request may close these issues.

2 participants