-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat(): fetch coverart images for releases #37
Conversation
0e81306
to
9823c41
Compare
Changing Release id and title to optional is not the way to go in my opinion. I don't see why the As far I understand coverart entities for release and release groups are the same right ? impl<'a, T> FetchCoverartQuery<T>
where
T: Clone + FetchCoverart<'a>
{
pub fn execute(&mut self) -> Result<Coverarts, Error>
// ... Also if we really want to had something to get the coverart from an entity we already got from the musicbrainz api, we could add another method to mutate the release/release-group entity : pub trait FetchCoverart<'a> {
fn id(&self) -> &str;
fn set_coverart(&mut self, coverarts: Coverarts) -> &mut Self;
fn get_coverarts(&mut self) -> Result<&mut Self, Error>
where Self: Sized + Path<'a> + Clone + Fetch<'a> + DeserializeOwned {
self.set_coverart(Self::fetch_coverart()
.id(self.id())
.execute()?);
Ok(self)
}
// ... This would allow to write something like this : // Fetch coverarts only
let in_utero_coverarts: Coverarts = Release::fetch_coverart()
.id("76df3287-6cda-33eb-8e9a-044b5e15ffdd")
.execute()
.expect("Unable to get cover art");
// Fetch release
let mut in_utero: Release = Release::fetch()
.id("76df3287-6cda-33eb-8e9a-044b5e15ffdd")
.execute()
.expect("Unable to get release");
// Add coverart
let in_utero_with_coverarts = in_utero
.get_coverarts().expect("Unable to get coverart");
assert!(in_utero_with_coverarts.coverarts.is_some()) Also you probably want to move the coverart struct to a dedicated module. |
9823c41
to
cecbc7f
Compare
cecbc7f
to
fc78ab0
Compare
This indeed sounds better than how I previously implemented it.
Yup, right. I've made an attempt on addressing these proposed changes. For now, I've implemented only one way to fetch the coverart, which looks like: let in_utero_coverarts: Coverarts = Release::fetch_coverart()
.id("76df3287-6cda-33eb-8e9a-044b5e15ffdd")
.execute()
.expect("Unable to get cover art"); If this looks good so far, I can attempt working on other ways to fetch the coverart. |
Great work, we shall see later if we feel the need to implement fetching coverart for fetched entities. |
Indeed, thanks!
By the way, I raised this on the #metabrainz IRC and this does seems like a bug with coverartarchive.org. |
This PR adds method(s) for fetching coverart for release entities. I have a few concerns:
I've had to change the type for
Release::ID
andRelease::Title
attributes to anOption
, since the coverart API doesn't return these values. Is this going to be okay? Since we do not treat these two attributes as anOption
for other entities.It seems like the coverart API returns an inconsistent response depending on the MBID. For example, the release entity
76df3287-6cda-33eb-8e9a-044b5e15ffdd
returns the value of the keyID
in the json response as an integer.https://coverartarchive.org/release/76df3287-6cda-33eb-8e9a-044b5e15ffdd?fmt=json
While, the release entity
18d4e9b4-9247-4b44-914a-8ddec3502103
treats the value ofID
as a string (in quotes) in the json response.https://coverartarchive.org/release/18d4e9b4-9247-4b44-914a-8ddec3502103?fmt=json
In this PR, I've assumed them as an integer but our json parser breaks in cases it receives a string for
ID
(since it expects an integer). Is this a bug in the web API and should be reported?I've yet to work on the builder pattern which looks like:
as you mentioned in #28 (comment).
I wanted to get some feedback before on the current work so far, whether or not is this the right way to implement coverart? What the current usage looks like can be seen from
tests/release/release_coverart.rs
.