search

oknozor / musicbrainz_rs

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

refactor(includes): split incs into variants #55

Closed ritiek closed 3 years ago

ritiek commented 3 years ago

I initially tried to implement an Include trait which would be carried to by Subquery and Relationship (musicbrainz docs). However since size for traits isn't known by the compiler at compile-time so I'm not sure how we could modify this to be considered valid code by the compiler. I think maybe Box<T> could help workaround this by allocating memory on the heap but I cannot seem to get it to work.

So I've used an enum based approach to deal with this at the moment. Not sure which is the better/best way though. I am open to any ideas.

Addresses https://github.com/oknozor/musicbrainz_rs/issues/9#issuecomment-867370925.

ritiek commented 3 years ago

I really don't see why we would use a trait here, this would indeed require dynamic dispatch (Box<dyn T>) while we can simply use a enum. I think the current implementation is fine.

I see, alright!

oknozor commented 3 years ago

I think we need to add some tests for all relationship includes before merging this.

ritiek commented 3 years ago

I think we need to add some tests for all relationship includes before merging this.

Alright, will do!

ritiek commented 3 years ago

It seems like we already had a possible test-case for artist-rels and event-rels. I split an existing test-case which had been testing two relation includes (artist-rels and event-rels) on Artist into two separate test-cases.

We had been missing tests for url-rels, so I added one each on Artist and Release.

I also tested on a few other entities - Recording and ReleaseGroup, it seems like musicbrainz always returns an empty relation list for them for the three relation includes currently implemented (artist-rels, event-rels and url-rels), and it seems to be the similar case for Artist entity when requesting for event-rels. So, it's probably not a good idea to implement such relation includes on these entities.

This should be ready to merge if everything looks good!

oknozor commented 3 years ago

Bravo for the hard work @ritiek :rocket: ! I've added you as a repo member, so we can get test coverage on your PRs, also it should be more comfortable for you to submit your PRs, rebase etc.