SpotifyAPI-NET icon indicating copy to clipboard operation
SpotifyAPI-NET copied to clipboard

Playlists.Get API should allow specifying fields, not force fields to empty list

Open ta264 opened this issue 3 years ago • 10 comments

See #470 and the fix in d73c87440038c463427e173265a36b0a839b769d

As far as I can tell you cannot actually set the Fields property as it's read only. So it gets forced to an empty list and you get very little back.

I'm trying to update Lidarr to use 6.x but this is currently blocking for me. Thanks in advance (and apologies if I'm doing something silly)!

ta264 avatar Jul 20 '22 12:07 ta264

The field is readonly, but you can still add fields with model.Fields.Add(...)

Although we may want to change the type to List, which would also allow to do AddRange(otherList)

JohnnyCrazy avatar Jul 20 '22 12:07 JohnnyCrazy

Thanks for the quick response! I didn't think of that.

I'd definitely be in favour of a List or a setter - this is my current field list: id, name, tracks.next, tracks.items(track(name, artists(id, name), album(id, album_type, name, release_date, release_date_precision, artists(id, name))))

ta264 avatar Jul 20 '22 13:07 ta264

Although having said that, that's only 4 calls, could be worse. Thanks again.

ta264 avatar Jul 20 '22 13:07 ta264

Yea, a List<T> would definitely make more sense

JohnnyCrazy avatar Jul 20 '22 13:07 JohnnyCrazy

In reference to #739, currently I think this setup means everything will be empty unless you explicitly ask for it. I think it might be more sensible to find a way to not pass through fields unless it's got something in it. If a user wants everything and has to manually enumerate all the possible fields that's pretty clunky.

ta264 avatar Jul 20 '22 14:07 ta264

I just tested this with the following call, and all fields are populated:

Paging<SimplePlaylist> playlists = await spotify.Playlists.CurrentUsers();
Debug.WriteLine(playlists.Items[0].Name);

image

JohnnyCrazy avatar Jul 20 '22 14:07 JohnnyCrazy

Ah I hadn't spotted the #739 was using CurrentUsers. That one sends a PlaylistCurrentUsers reques that doesn't let you set fields https://github.com/JohnnyCrazy/SpotifyAPI-NET/blob/b400d63087ce0eb78cd73b59a16cfd3fbf4f5907/SpotifyAPI.Web/Models/Request/PlaylistCurrentUsersRequest.cs

I think my concern above would still apply on the Playlists.Get methods, which will default to an empty field list I think.

ta264 avatar Jul 20 '22 14:07 ta264

I'm very confused how you would come to that conclusion :D

image

Fields are not applied if they're empty: https://github.com/JohnnyCrazy/SpotifyAPI-NET/blob/7acffd96bab8111eeb59f934c434730e53afa8ca/SpotifyAPI.Web/Models/Request/RequestParams.cs#L106-L115

JohnnyCrazy avatar Jul 20 '22 15:07 JohnnyCrazy

Haha sorry, I think I must have some other bug in Lidarr causing me grief. Sorry again for the noise, let me dig into it my side.

PS great work on the library, and (coming from 4.x) it does seem like a big improvement even if I have some wrinkles to work out 😁

ta264 avatar Jul 20 '22 15:07 ta264

No worries, feel free to ask more in a seperate discussion if stuff comes up :)

JohnnyCrazy avatar Jul 20 '22 15:07 JohnnyCrazy