-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
MetaSync: more OO structure + iTunes support #1450
Conversation
Uses plistlib to read a temp copy of `iTunes Library.xml`
changelog & metasync plugin documentation
|
||
The path to your iTunes library **xml** file has to be configured, e.g.:: | ||
|
||
metaysnc: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metaysnc => metasync
- allows for the logging to integrate better - renamed `get_data` to `sync_data`, to better reflect that its not returning anything, but that the item's data is being set by the function
- MetaSources get loaded from the modules automatically - The MetaSources can define their own item_types, that get loaded for the plugin - __init__ doesn't need any changes to accept new metasources - Fix the --sources option to actually accept sources (it was being interpreted as boolean flag before, crashing the plugin) - More safety w.r.t. external dependencies
@pprkut Thats' a good point; I never considered reverse syncing (which definitely would need its own function). What do you think about |
Generally fine, but I don't think 'syncing to source' would be semantically correct ;-) How about |
I'll stick to |
- `sync_data` -> `sync_from_source` - properly catch ConfigValueError - avoiding iterating through library if we couldn't instantiate any meta sources - fix create_temporary_copy to actually make a tempdir
No tests for Amarok, unfortunately
I think this is done now. I tested it on a Windows machine with iTunes and all seems good. Maybe @pprkut could add some tests later on for Amarok? There could still be future work to move the |
for cls_name, _cls in classes: | ||
meta_sources[cls_name.lower()] = _cls | ||
|
||
return meta_sources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is some very clever work to get auto-discovery in order! It could be, though, that it would be easier just to hard-code the list of modules to import and pull classes out of—this sort of pkgutil
and inspect
magic could make this harder to maintain in the future. (We already run into some amount of trouble with testing, for example, when it interacts with similar magic in the beets plugin loading system.)
So I'll advocate gently for the "dumb" solution here: just redundantly writing down the list of available backends rather than automatically discovering them. But if anyone thinks this is too nifty to throw away, I could be convinced. 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I should mention I realize that you didn't invent the magic here; it's just clearer this time, so it's the first time I thought about it carefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose you are right, considering there won't be that many sources.
Think I was just having some fun writing the auto-discovery, and sort of forgot the whole "Simple is better than complex" 😃
This is looking really great, @tomjaspers. This backend is one I might use myself. I have a few lower-level comments above; please feel free to ask for clarification on anything. |
In favor of simpler, hard-coded, list of sources, to avoid unneccesary magic. Also: check to see if query has results before instantiating the meta sources
- Use path as an key to find items (over artist/title/album tuples) - Sensible default library location
@sampsyo I've made the changes. I'm not exactly sure why Travis is failing, though? There were quite some quirks with iTunes' way of storing the path, but they're documented in the Anyway, I'll be away for a few days, but can work on it some more if need be, next week. |
Awesome. This looks really cool; nice hacks to get the paths in shape. And yes, I don't know what's up with Travis. The failures seem unrelated to this plugin. I suspect something wrong with the Travis environment, but they haven't changed the build environments lately: http://docs.travis-ci.com/user/build-environment-updates/ Investigating. |
MetaSync: more OO structure + iTunes support
Any plans to update this plugin for the Apple Music app? |
Adds support for syncing iTunes metadata: rating, play count, skip count, last played, last skipped
It uses
plistlib
, which is available for all platforms (since py26), but the behavior is completely untested on Windows system (only tested on my OS X system). The iTunes library is copied to a temporary file before reading it, to avoid corrupting it.Want to do a few more things, but figured I'd get it up here already (since it technically works).
FetchArt
andArtSources
)item_types
from the MetaSources