zmb3 / spotify

A Go wrapper for the Spotify Web API
Apache License 2.0
1.41k stars 295 forks source link

fix: unmarshal numeric fields regardless of them being int or float #251

Closed wernerdweight closed 8 months ago

wernerdweight commented 9 months ago

This fixes #243. I also updated the test data so that it contains float values.

kreudom commented 9 months ago

This does not work for me for the FullTrack. The fields stay zero.

I think the problem is here: https://github.com/zmb3/spotify/blob/f5f7cf5e1671692f8dace79d61562e48d5d60914/track.go#L141-L148

Because SimpleTrack implements UnmarshalJSON, the struct v also implements it. Therefore json.Unmarshal directly calls UnmarshalJSON and ignores the other fields.

strideynet commented 9 months ago

Thanks for raising this - could you address my remark re: the test data ? I think it's important to prove we can handle both ints and floats. Then I'm happy to move forward with this.

wernerdweight commented 8 months ago

@strideynet, I rewrote it with the custom type to avoid multiple unmarshallers. It's much simpler and flexible now. That said, it introduces a breaking change for comparisons. So if you, for some reason, compare one of the fields (e.g. the follower count) to an integer variable, it will break, as the custom type is not comparable to the int.

playlist.Followers > 100 // still works
threshold := 100
playlist.Followers > threshold // doesn't work (mismatched types Numeric and int)

I still think this is a better solution than chaining the unmarshallers on top of each other, but since this is a bc-break, we/you could consider switching to floats (which would actually be a small change now that there's a custom type). Thanks

EDIT: it's a bc-break invoked by a bc-break by Spotify, so maybe not a big deal (bc-break vs. doesn't work).

strideynet commented 8 months ago

I still think this is a better solution than chaining the unmarshallers on top of each other, but since this is a bc-break, we/you could consider switching to floats (which would actually be a small change now that there's a custom type).

Thanks - I think I'm happy enough with this hack for now. I'll cut a release with a breaking change warning.