Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed currently_playing to return nil if the response does not contain an item. #147

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Changed currently_playing to return nil if the response does not contain an item. #147

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7893a91
Added nil check to User.currently_playing
owen-roberts Nov 23, 2017
c249159
Fixed nil check
owen-roberts Nov 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/rspotify/user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about using fetch to accomplish this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very new to Ruby so wasn't aware of this, looks interesting though so will take a look at adding it :)

Copy link
Author

@owen-roberts owen-roberts Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally, but checking for a specific class is better than checking for nil I think. 🤷‍♂️ ruby problems.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return response if RSpotify.raw_response
Track.new response["item"]
end
Expand Down