search

Bogpan / spotify-rs

Rust wrapper for the Spotify API.
https://crates.io/crates/spotify-rs
Apache License 2.0
10 stars 5 forks source link

AudioFeatures return type #11

Open Multimodcrafter opened 2 months ago

Multimodcrafter commented 2 months ago

I just discovered that the documentation of the spotify api is not quite correct concerning the 'Get Several Tracks' Audio Features'. Namely, the returned result can contain null values when there are no audio features for a certain track.

This can easily be fixed by changing the return type to an Option<_>.

Be aware that the way I changed it is non-backwards compatible, so it's probably not good to merge it blindly. I still wanted to let you know and point out the necessary change. An easy way to make it backwards compatible is to add a new function which returns the correct result.

Bogpan commented 2 months ago

Funny you mention that, I've realised it too quite a while ago. In the rewrite I first filtered null values out and then changed the field to a Vec<Option>. I'm still not sure how to proceed with this, as Vec<Option> is not as nice to use, but it's more transparent than filtering out the null values.

So I take it you think using an Option is the best way? Thanks for the pull request, by the way. I'll merge it into the rewrite branch once I tidy it up a bit and release it on GitHub.

Multimodcrafter commented 2 months ago

Yes, I'd argue for going the Option<> route. The way I use it atm it doesn't matter if null values are filtered, but I'm planing on a different way to use the result in which it will be very important to know which ones are null.

It's very easy to get the filtered version if you have a vec of options by simply doing a filter map like this:

vec.iter().filter_map(|obj| obj?).collect()

This transforms a vector of options into a vector of all 'Some' values, effectively filtering out the null values from the api result.

I'd say this is convenient enough, but of course, you could provide two seperate functions if you think the use of the filtered result is important enough :)

Bogpan commented 2 months ago

I see, thanks for the input and the PR!I'll go the Vec<Option> route then. I'll also try to release the rewrite to GitHub soon and merge the PR!

Multimodcrafter commented 2 months ago

Btw, why haven't you pushed your current progress of the rewrite on a seperate branch? Just curious :)

Bogpan commented 2 months ago

At first I thought I'd just release it directly but I'd rather publish something usable even though it's not perfect and make a few breaking changes on that branch possibly. And I haven't done that yet because it's messy, there are lots of "stupid" todos and comments haha

Bogpan commented 2 months ago

next was just published. I'll deal with this PR later!