Batch scrobbling and Scrobbler classes

[rikkit]

Tracking issue for the new auto-caching + batching scrobbler

TODO

• Base Scrobbler classes
• SQLite Scrobbler class
• Tests
• Support multiple scrobbles on ScrobbleCommand
• Batch up scrobbles in ScrobblerBase
• Test batching and caching in unit tests
• Remove extra nugets from SQLite lib
• SQLite nuget package

[rikkit]

@Zumicts yo. Do you think this pattern makes sense for you? The intention is that this feature (batching + caching) can be integrated in pretty much one line of code. The core includes a basic non-caching scrobbler (Scrobbler.cs), then scrobblers that use database packages can be distributed separately (the SQLiteScrobbler class in this PR will be distributed in Inflatable.Lastfm.SQLite for example).

Theoretical code:

_scrobbler = new SQLiteScrobbler(auth, databaseFile);
var response = _scrobbler.ScrobbleAsync(track.AsScrobble(DateTimeOffset.Now));

// if _scrobbler.CacheEnabled == true then response.Status will be LastResponseStatus.Cached or LastResponseStatus.CacheFailed, otherwise will be the status of the internal scrobble HTTP request

Related issue; I notice in Audiotica you have wrapped calls to the SDK in extra service classes, I'd like for that to be unnecessary if possible to reduce lines of code. So you'd DI ITrackApi directly. Would like your thoughts on that too.

[haroldma-zz]

I think it makes sense, follows the SOLID pattern and in the future it makes it easier to implement other cache scrobblers. All though maybe we can break the cache functionality away from the Scrobbler class and just have something like Scrobbler.SetCache(IScrobblerCache:cache) or pass it in the constructor (more DI friendly).

Yeah, in Audiotica all the related class I use for last.fm are injected.

[rikkit]

Good to hear. Feature should be in master in the next couple days. 👍

Yeah, in Audiotica all the related class I use for last.fm are injected.

I was talking about IScrobblerService specifically. Having a single class to DI is definitely better than seven. Does this class do anything special? If not we can make something like it in Core.

[haroldma-zz]

Oh, yeah it is definitely better to have one class that interacts with the api. The class does not implement anything special. Actually, I think it is using my credential helper class for storing username and password but that can easily be abstracted.

EDIT:

These rely on the cred helper and should be remove if you plan on distributing it:

 event EventHandler<BoolEventArgs> AuthStateChanged;
 bool HasCredentials { get; }
 bool IsAuthenticated { get; }
 void Logout();

[rikkit]

So I'm thinking of abusing properties a bit and making a super-api class which composes the existing ones.

So the README example

var auth = new LastAuth("apikey", "apisecret");

// wait for authentication
var response = await auth.GetSessionTokenAsync("username", "pass");

if (response.Success && auth.HasAuthenticated) {
    var trackApi = new TrackApi(auth);
    var loved = await trackApi.LoveTrackAsync("CIRCLONT6A [141.98][Syrobonkus mix]", "Aphex Twin");
}

becomes

var lastApi = new SuperLastApi(apikey, apisecret, httpclient, scrobbler);

var response = lastApi.Auth.GetSessionTokenAsync();
if (response.Success && lastApi.Auth.HasAuthenticated)
{
var loved = lastApi.Track.LoveAsync("CIRCLONT6A [141.98][Syrobonkus mix]", "Aphex Twin");
}

This'd be flexible to both approaches whilst keeping the codebase tidy. It mirrors the api method names nicely too.

[haroldma-zz]

That seems like a nice way of doing it. Would you use DI, or do some lazy loading like:

public Auth Auth
{
      get
      {
          return _auth ?? (_auth = new Auth());
      }
}    

[rikkit]

I don't think it's worth implementing lazy loading since the Super API class will only ever hold around 10 object references. The API classes themselves don't store any references, apart from LastAuth (strings and UserSession), and HttpClient if that is provided.

public SuperLastApi(string key, string secret, HttpClient httpClient = null, IScrobbler scrobbler = null)
{
    var client = httpClient ?? new HttpClient(); // TODO copy options from command base

    Auth = new LastAuth(key, secret, client);
    Scrobbler = scrobbler ?? new Scrobbler(client);
    Track = new TrackApi(client);
    Album = new AlbumApi(client);
    Artist = new ArtistApi(client);
    Library = new LibraryApi(client);
    // etc.
}

[rikkit]

All though maybe we can break the cache functionality away from the Scrobbler class and just have something like Scrobbler.SetCache(IScrobblerCache:cache) or pass it in the constructor (more DI friendly).

Didn't see this edit earlier.

This depends on whether there are other kinds of things we can do with a database in the SDK; ILastDbProvider and LastSQLiteDbProvider will definitely be the way to go if so; right now though all I can think of is error logging, which I don't want to implement in this release (0.2 is already way overdue lol). I think the current derived class approach is friendly enough to both DI and non-DI apps for now and can always revisit the issue in the future.

[haroldma-zz]

I guess you're right but should it implement IDisposable because of the HttpClient (don't want a memory leak)?

Ok, let's keep what we have and improve as we go, after all this is just 0.2 and we have a long way to 1.0 😆

[haroldma-zz]

Now that I think of it, all commands should be disposable since each one is using an HttpClient but in the SuperLastApi we will only disposed of the client that was passed to each command.

[rikkit]

I guess you're right but should it implement IDisposable because of the HttpClient (don't want a memory leak)?

Yuuuuuuup. >_>

Now that I think of it, all commands should be disposable since each one is using an HttpClient but in the SuperLastApi we will only disposed of the client that was passed to each command.

#41 covers this, commands created inside a *Api class will use the same HttpClient. *Api classes should be disposable though. I'll do that as part of this PR