oknozor / musicbrainz_rs

A wrapper around the musicbrainz API
MIT License
38 stars 18 forks source link

feat(coverart): coverart builder to make specific queries to coverart api #45

Closed ritiek closed 3 years ago

ritiek commented 3 years ago

Ideally this will allow us to make specific calls to the coverart api. Such as to fetch front coverart for some release in 500px:

// https://coverartarchive.org/release/76df3287-6cda-33eb-8e9a-044b5e15ffdd/front-500
let in_utero_coverart = Release::fetch_coverart()
    .id("76df3287-6cda-33eb-8e9a-044b5e15ffdd")
    .front()
    .res_500()
    .execute()
    .expect("Unable to get cover art");

I'm not sure what the return type in our library should be here. The coverart api simply returns an image for this query.

Should we also return an image (maybe through the image crate), or maybe just return the redirect url from the original query (which looks like https://ia802607.us.archive.org/32/items/mbid-76df3287-6cda-33eb-8e9a-044b5e15ffdd/mbid-76df3287-6cda-33eb-8e9a-044b5e15ffdd-829521842_thumb500.jpg)?

Also, currently FetchCoverartQuery::execute() expects response content to be in json, but making these specific coverart calls does not return json. Not sure what the best way to deal with this in our library is.

oknozor commented 3 years ago

Hello @ritiek I won't have time to review this before next week.

Also I had a message from Yvan telling me that we need to give sign of life if we want to continue in the gsoc program.

I will contact you next week.

ritiek commented 3 years ago

Hello @ritiek I won't have time to review this before next week.

I see. Hope you're doing fine. Is there anything else you'd specifically want to me to work on until then? Otherwise I plan on implementing auto-retries for now as this feels something I wouldn't require much consulting on.

Also I had a message from Yvan telling me that we need to give sign of life if we want to continue in the gsoc program.

Totally understandable where this is coming from.

oknozor commented 3 years ago

Everything is fine, I am just super busy these days. Go for auto-retries !

ritiek commented 3 years ago

Good to know! Alright.

ritiek commented 3 years ago

An alternative is to push the parameters (front, back and res) as some enum variant and check just before sending the request if all params are compatible.

Sounds good. How should I validate the request before sending it? Should I create another method (validate()?) that gets called as a part of execute() and checks whether the parameters are ordered correctly and are without repetition?

oknozor commented 3 years ago

Calling a validate() function as part of execute would do it, I don't think you need to check how method call are ordered since we can reorder everything when building the request. You only need to check for repetition.

ritiek commented 3 years ago

Alright. I've pushed new changes. What do you think?

oknozor commented 3 years ago

Also we need to update the readme.

ritiek commented 3 years ago

Also we need to update the readme.

Ok. Should I mention about the coverart feature in the features section in readme? And is it okay if I do this in a later PR? I'll also cover about get_coverart method and anything else worth mentioning about (retries?) in a later dedicated PR, if it's alright to you.

ritiek commented 3 years ago

Is there anything else that needs to be addressed? Otherwise, this should be good to merge!