Conversation
Merging this PR will not alter performance
Comparing 
Footnotes
|
35a79b4 to
c1a4ab3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Disabling podcast synchronization now also disables browsing and playback, and the unrelated dependency bump should be separated.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Removes the OpenSubsonic-specific podcast option in favor of the shared library-sync setting.
Changes:
- Removes the provider-specific podcast configuration and translations.
- Uses the generic podcast sync setting.
- Updates py-opensonic to 10.4.0.
File summaries
| File | Description |
|---|---|
requirements_all.txt |
Updates py-opensonic dependency. |
music_assistant/translations/en.json |
Removes generated podcast-option strings. |
music_assistant/providers/opensubsonic/strings.json |
Removes source translation strings. |
music_assistant/providers/opensubsonic/sonic_provider.py |
Replaces the podcast option with the sync setting. |
music_assistant/providers/opensubsonic/manifest.json |
Updates the provider dependency. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| self.logger.info("Failed to query server for OpenSubsonic extensions") | ||
|
|
||
| self._enable_podcasts = bool(self.config.get_value(CONF_ENABLE_PODCASTS)) | ||
| self._enable_podcasts = bool(self.config.get_value("library_sync_podcasts")) |
There was a problem hiding this comment.
To the best of my knowledge we have precisely 0 users who would want to see podcast entries from the Open Subsonic server but not have them synchronized to MA for playback. Maintaining both toggles led at least one user to hit an unexpected failure when their implementation return 501 from the getPodcasts endpoint.
I can alter the PR to remove the _enable_podcasts checks but then we are handling exceptions from py-opensonic if a user ever goes down those paths. However, the check for the recommendations stays. I don't see a use case for wanting to see the latest podcast episodes when we are not syncing them for playback.
There was a problem hiding this comment.
Why do we have this toggle at all? Why not just enable it always? If there are podcasts to sync, users want them in MA right? Or am I missing something here?
There was a problem hiding this comment.
Only 1-2 of the Open Subsonic implementations we see support podcasts at all (including the one I use). This avoids making calls that we know are not implemented. The spec does not define the "correct" thing to do when an endpoint is not implemented, IIRC I have seen multiple different ways. I have seen http 404, 501, or subsonic errors come back from various implementations. Rather than having fiddly error handling that attempts to differentiate "I don't implement this" from "Something is actually wrong", we have avoided making the call when users know that podcasts are not supported/desired.
5f08f23 to
a03278a
Compare
|
Please ignore the update to the dependency library here, github is stupid about managing PR dependencies. There is a separate PR for that update that is tagged for back porting to stable. |
Use the more generic option from MA instead. Having two has caused user confusion. Signed-off-by: Eric B Munson <eric@munsonfam.org>
a03278a to
b71a385
Compare
What does this implement/fix?
The Open Subsonic provider has an option to enable/disable podcast syncing that conflicts with the more generic library sync option. We don't need both so remove the provider specific one and just use the library sync option.
Types of changes
bugfixnew-featureenhancementnew-providerbreaking-changerefactordocumentationmaintenancecidependenciesChecklist
pre-commit run --all-filespasses.pytestpasses, and tests have been added/updated undertests/where applicable.