Favorites management API changes#127
Conversation
…gement Signed-off-by: darrell-k <darrell@darrell.org.uk>
Signed-off-by: darrell-k <darrell@darrell.org.uk>
|
Running with the changes now. Do you anticipate this having any affect on the OMLI import process? I haven't looked at the code yet but could imagine a possible conflict. |
|
There shouldn't be any effect on OMLI, but worth checking. |
| $self->getUserFavorites(sub{}, 'refresh'); | ||
| $self->getUserFavoriteIds(sub{}, 'refresh'); | ||
| $self->getUserFavoriteStatus(sub{}, { type => 'album', item_id => $args->{album_ids} }, 'refresh') if $args->{album_ids}; | ||
| $self->getUserFavoriteStatus(sub{}, { type => 'artist', item_id => $args->{artist_ids} }, 'refresh') if $args->{artist_ids}; | ||
| $self->getUserFavoriteStatus(sub{}, { type => 'track', item_id => $args->{track_ids} }, 'refresh') if $args->{track_ids}; |
There was a problem hiding this comment.
This is doing more work than before. Is it worth it? Wouldn't the first call have all the information the subsequent two would get us?
There was a problem hiding this comment.
We're just clearing caches here, after an add or delete favourite. Maybe we should do it within the add/delete api routines. At least we would know which of the 3 types to clear in the case of the status cache.
There was a problem hiding this comment.
This is not only wiping caches, you're doing calls to eg. favorite/status, too, right?
There was a problem hiding this comment.
Yes, sorry I meant wipe/recreate cache. We could delay the recreation of the cache entry until it is requested again, so just do the clear at that stage.
Signed-off-by: darrell-k <darrell@darrell.org.uk>
|
See new commit. I don't think it's worth refreshing the favorites caches when favourites change when they only last 60 seconds anyway, so we just delete them. |
|
|
||
| if ($params->{_wipecache}) { | ||
| $cache->remove($cacheKey); | ||
| if ( $params->{_wipecache} eq 'delete_cache_no_refresh' ) { |
There was a problem hiding this comment.
As we are dealing with named parameters in $params, why not pass $params->{delete_cache_no_refresh} instead of doing a string comparison, rendering _wipecache ambiguous? At the very least I'd strongly recommend you replace the repeated use of the string literal with a constant. That way a typo would immediately cause the start to fail, rather than fail the comparison at run time, leading to unexpected behaviour.
There was a problem hiding this comment.
See latest commit. I've changed the favorites APIs to use only named parameters. I've also allowed for callers to override any parameters defined in the individual APIs if they want to.
Do you like this approach? Should I extend it to all the APIs?
There was a problem hiding this comment.
I'd use constants whenever there's static string literals to compare otherwise. If you know of other places where this happens I'd recommend you change that. I only commented on the changes in this PR...
There was a problem hiding this comment.
I'll check for string literals.
What I was trying to say is that the APIs use a mixture of positional parameters and named parameters in a hash. I've changed this for the favorite* APIs so that named parameters are always used.
I was wondering if I should apply the same to the rest of the APIs?
Signed-off-by: darrell-k <darrell@darrell.org.uk>
3 participants
Replace favorite/getUserFavorites with favorite/getUserFavoriteIds or favorite/status when retrieving the status of favorites. It's a lot less data.
In
QobuzGetTracksandQobuzArtistuse favorite/status to return a simple boolean.In
QobuzManageFavoritesuse favorite/getUserFavoriteIds because although it's more data to return than favorite/status, it avoids having to call the status API 3 times (for albums, tracks and artists).The new API results are cached for 60 seconds, like favorite/getUserFavorites, so extend the cache management in
createFavoriteanddeleteFavorite.Please give this a good going over.