Search with thumbnails #5
Conversation
This implementation follows the API a bit closer, and exposes a way to make more complex searches using this package.
Normally I leave it up to the packages or projects using the integration package to "interpret" the underlying API, but for the sake of this example, this could also partially be handled by a new helper class. For one, the helper class contains the "GetVideoByMediaId" and "GetVideoByUniqueMediaId" methods. These don't really exist in the underlying API, so they are really just calling the "Search" method with the right parameters, delegating the work to the underlying parts of the package.
…ions.cs Co-authored-by: Jesper Mayntzhusen <[email protected]>
There was a problem hiding this comment.
Hi @jemayn
I'm still not a super fan of all the logic in the client and service classes.
Right now we're also requesting a new token from inside the constructor of the SkyfishHttpClient class. Doing something like this inside a constructor is generally considered bad practice.
So at least for the token logic, we can do something like:
- Remove
_token = GetToken();from the constructor - Make the
GetTokenmethod public, but move the cache logic somewhere else - Make the
GetTokenmed return theSkyfishAuthResponse - Introduce a new static
CreateWithToken(string apikey, string secretkey, string username, string password)method that initializes the client and fetches the token - The
SkyfishAuthResponsename may also be a bit misleading, so it should probably be something likeSkyfishTokeninstead
Like I wrote in one of the other reviews, I'm also not a big fan of having the cache in the integration package, but if we are creating a new CreateWithToken method, maybe we could also have a CreateWithTokenMemoryCached method that does the same, but with the memory cache logic (feel free to come up with some better names).
And maybe base the memory key on the username - or a hash of the four parameters. Then the code doesn't prevent us from using multiple Skyfish accounts should we need that in the future.
|
Since the changes in this PR is a bit urgent for a client project, we're merging the PR now, and then making the implementation a bit prettier later. |
Since the new search endpoints return a
SkyfishMediaItemwhich has exactly the same properties as theSkyfishVideomodel I've removed the old model, kept the new and changed theGetVideoendpoint to just be for getting the embed url instead (that is also where all the weird logic was anyways).So this works together with the changes pushed to skybrud/Skybrud.VideoPicker.Skyfish#1, and now works more like an integration package that has methods for specific functionality instead of combining a bunch of calls to return a full video object in the end.