Changed currently_playing to return nil if the response does not contain an item. #147
Conversation
| @@ -118,6 +118,7 @@ def create_playlist!(name, public: true) | |||
| def currently_playing | |||
| url = "me/player/currently-playing" | |||
| response = RSpotify.resolve_auth_request(@id, url) | |||
| return nil unless response && response['item'] | |||
There was a problem hiding this comment.
What do you think about using fetch to accomplish this?
There was a problem hiding this comment.
Very new to Ruby so wasn't aware of this, looks interesting though so will take a look at adding it :)
There was a problem hiding this comment.
Part of the problem seems to be that response itself is coming back as nil, so I can't do response.fetch because that also throws a undefined method for nil error.
I could change response['item'] to use fetch, but the unless response bit needs to stay as far as I can tell.
There was a problem hiding this comment.
Ah, of course, I should have known that. It would be nice to examine the code paths that are returning nil here in the first place, since that is the real root of this evil.
There was a problem hiding this comment.
I just dug a little bit. This is just my personal opinion, it may not actually be useful, but I would think a nice way to handle this would be to return some kind of null object from here when the response is nil or empty.
There was a problem hiding this comment.
Yup, agree completely. Looks like the send_request is used by all (or most) of the other methods so that will fix this problem across the board rather than just for the currently playing track.
I've also found a bug with character encoding that I needed to change the send_request method for, so I may include that in a future pull request as well.
There was a problem hiding this comment.
Just realising a slight problem with that, surely returning a null object from send_request is still going to require a check in currently_playing (or any other method in user) to determine if the response has a value or is a null object, which would essentially be the same as what the nil check is doing? Otherwise at some point Json is going to try parse whatever response is, and if it isn't able to that's going to cause problems.
There was a problem hiding this comment.
Totally, but checking for a specific class is better than checking for nil I think. 🤷♂️ ruby problems.
There was a problem hiding this comment.
We could also have a NullObject that returns empty collections for the things it is supposed to have, if that makes sense. So response[item] could return '' or something like that.
Ran in to problems where using currently playing would result in
undefined method '[]' for nil:NilClasserror if no track was being played.This appears to resolve that problem and enabled the ability to check for nil when getting the current track.