search

oknozor / musicbrainz_rs

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

feat(includes): support for additional relationship includes #60

Closed ritiek closed 3 years ago

ritiek commented 3 years ago

This PR adds support for most missing relationship includes.

I've been trying out possible relation includes on all the major entities in musicbrainz, and implementing the ones that are accepted by the musicbrainz web-api.

Currently, relationship includes have been implemented for these entities:

I'll mark this PR ready for review once I am done with all of them!

Addresses #9.

ritiek commented 3 years ago

Seems like one of the tests is timing out with cargo tarpaulin (taking >1 minute), probably because of the deserializer taking a lot of time to parse the large api response. I'll see if I can find a different area entity which has a smaller api response. Otherwise we'll have to pass a higher timeout limit to tarpaulin (-t 150 works fine for me locally) or skip this test altogether in the CI.

ritiek commented 3 years ago

Ok, so I switched the Area MBID on the test-case that had been timing-out with the one that had a smaller api response and the CI seems to be working fine now!

ritiek commented 3 years ago

There are some tests that are panicking (but ideally should not), so I've commented them with FIXME for the moment. I plan to take a look at those next week (probably) maybe in a separate PR.

Otherwise this should be ready for review!

ritiek commented 3 years ago

I've rebased this.

ritiek commented 3 years ago

(a gentle ping @oknozor, GSoC evaluations are out and the deadline is today (16th) if you haven't already filled it out!)

oknozor commented 3 years ago

@ritiek I just completed the evaluation, it's mostly positive :). However I think we should both attend the monday meeting next week. Would you be available ?

ritiek commented 3 years ago

Alright, good to know! ^^

However I think we should both attend the monday meeting next week. Would you be available ?

Yep, I'll be available.

oknozor commented 3 years ago

Hey @ritiek , sorry for the delay, there is some conflict to solve since I merged your other PRs

ritiek commented 3 years ago

@oknozor No problem! I've rebased this (hopefully correctly), so should be ready for review!