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

Chore: Audiobookshelf - adapt schema to reflect the naming scheme used in the API docs #1898

Merged
merged 5 commits into from
Jan 21, 2025

Conversation

fmunkes
Copy link
Contributor

@fmunkes fmunkes commented Jan 20, 2025

This PR adapts classes defined in abs_schema.py to use the same names (+ prefix ABS) as in the API doc of Audiobookshelf. Many items have multiple variants, e.g. a Book exists in a minified, expanded and normal version. Now, classes are named accordingly:

  • ABSBook
  • ABSBookMinified
  • ABSBookExpanded

I've been rather loose in the initial version, however, I believe this makes future maintenance and possible enhancements much easier. There is no logical code change in this PR.

@fmunkes
Copy link
Contributor Author

fmunkes commented Jan 20, 2025

that was bad luck - abs got updated to 2.18.0, and they changed some structure apparently. audiobooks appear to befine, podcasts not. I'm looking into this right now

Copy link
Member

@marcelveldt marcelveldt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@marcelveldt
Copy link
Member

that was bad luck - abs got updated to 2.18.0, and they changed some structure apparently. audiobooks appear to befine, podcasts not. I'm looking into this right now

Ah saw this too late - lets hold off merging then. Please mark as ready when its ready again

@marcelveldt marcelveldt marked this pull request as draft January 20, 2025 20:54
@fmunkes
Copy link
Contributor Author

fmunkes commented Jan 20, 2025

Found it. Sorry about that. Just updated the abs container, and then noticed the issue. It appears to be fine again, was just a minor issue.
@OzGav would you please add something like this to the documentation:
"This provider is always tested against the latest containerized non-development version of Audiobookshelf, i.e. ghcr.io/advplyr/audiobookshelf:latest"

@fmunkes fmunkes requested a review from marcelveldt January 20, 2025 21:15
@fmunkes fmunkes marked this pull request as ready for review January 20, 2025 21:24
@Jc2k
Copy link
Contributor

Jc2k commented Jan 21, 2025

Doesn't block this but would be good to add some tests for the schemas and the parse* functions in the provider - you can see how we do that in here: #1859. We are doing snapshot testing, and take a directory of sample JSON data from the target service and verify it against a snapshot of our music_assistant_models like Track, Album, etc.

(If you had those tests know, then you'd be able to refactor the schemas and know that you hadn't broken the Music Assistant models that are created in your parse* functions).

I can help with that, but I don't have any sample data on my end.

@fmunkes
Copy link
Contributor Author

fmunkes commented Jan 21, 2025

Doesn't block this but would be good to add some tests for the schemas and the parse* functions in the provider - you can see how we do that in here: #1859. We are doing snapshot testing, and take a directory of sample JSON data from the target service and verify it against a snapshot of our music_assistant_models like Track, Album, etc.

(If you had those tests know, then you'd be able to refactor the schemas and know that you hadn't broken the Music Assistant models that are created in your parse* functions).

I can help with that, but I don't have any sample data on my end.

That indeed sounds like a very good idea, thanks for offering your help!
I will look into it, though I will need some more time than I have on this weeks workdays, as this is completely new to me. I will invest some time this weekend to read up/ setup my environment for it, and would also create a test instance of abs, such that I can generate some useful json responses for e.g. an audiobook with very little/ a lot of metadata etc. Currently it's just tested against my normal instance.
When I have all this, I'd create a draft PR, where we can discuss how far I got. Is that ok with you?

@Jc2k
Copy link
Contributor

Jc2k commented Jan 21, 2025

That sounds great!

trying to make it more consistent with api docs. still wip
The naming of the classes is now the same as in the Audiobookshelf
API doc.
@marcelveldt marcelveldt force-pushed the abs_reorganize_schema branch from 26bd964 to 8cc6f16 Compare January 21, 2025 19:02
@marcelveldt marcelveldt merged commit 1b1bdec into music-assistant:dev Jan 21, 2025
4 of 5 checks passed
@fmunkes fmunkes deleted the abs_reorganize_schema branch January 21, 2025 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants