Feat/sync artists albums #92
Conversation
There was a problem hiding this comment.
I think this matching algorithm of just looking at the name would be too error prone to be used reliably. When matching strings like this you generally have to use multiple fields in order to avoid false positives and false negatives, as per the match() function. For albums you should also use the ipc code if it exists
|
I've improved the artist and album syncing. When syncing a users' album, the algorithm preferable uses the UPC code to retrieve it from the tidal API, if none is present it uses the search function with a query string and afterwards verifies the name, the artist and the number of tracks (I decided not to include the check for the release date as this reduced the syncing quality as for some reason some albums don't have the same release date as on Spotify...). The artist syncing matches artists by retrieving a few albums of the artist from the Spotify API. Afterwards it searches these albums on tidal and checks whether the artists are the correct one. I've also added a fallback to match an artist by their top tracks if none albums are available. |
|
If that works for you, I can create a cleanup commit. |
|
Sure that's a reasonable starting point, I'd need to go through and do some testing to find the right balance between speed and accuracy though. In general the PR needs quite a lot of cleaning up so if you could do your best to clean up, simplify, refactor, and match the existing style that would be appreciated, then I can take over the PR. |
src/spotify_to_tidal/sync.py
Outdated
| try: | ||
| albums = spotify_session.artist_albums(artist_id, album_type="album,single", limit=5)['items'] | ||
| return albums | ||
| except Exception as e: |
There was a problem hiding this comment.
In general it's very unlikely that you should need to catch a general Exception in a function like this. If there's a specific exception that needs to be handled here then please catch that specific exception. If not then just remove the try block.
There was a problem hiding this comment.
I would expect a maximum of ONE instance of catching except Exception as e for the entire project
There was a problem hiding this comment.
Yeah, that right. I should have thrown a more specific exception like spotipy.exceptions.SpotifyException. But I removed this function in my last commit.
There was a problem hiding this comment.
There are still 6 other instances of catching Exception though ;)
There was a problem hiding this comment.
I refactored the code to remove most of the try catch blocks. The one remaining in search_album_on_tidal is needed as the method get_albums_by_barcode is raising an ObjectNotFound exception when it cannot find an album with the given UPC.
|
Btw have you tested this? |
|
Yes, I have tested this and can confirm that both the artists and the album syncing do work. I've did some clean up, improved the artist syncing and reduced the number of methods. |
|
@timrae does this look good to you now? |
|
@jlindbergh can you look into this and merge the PR? |
Hi! Unfortunately I don't have time to look deeper into this, nor have I the privileges to approve a merge. It does look pretty reasonable to me though (without having really tested it...). Not sure what happens with saved spotify sessions when the |
|
Hi @jlindbergh, |
|
Sorry I haven't had capacity to look into this. Currently I'm the only maintainer, and am going to be pretty occupied with other projects for at least the next few months. I'd say continuing in your fork for now with the goal of merging back at a later point would be pragmatic for now |
No description provided.