8000 Quickfix Bug #7384 by DavidLP · Pull Request #7582 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Quickfix Bug #7384 #7582

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

Jump to bottom
Merged
merged 3 commits into from
Jun 2, 2017
Merged

Quickfix Bug #7384 #7582

merged 3 commits into from
Jun 2, 2017

Conversation

DavidLP
Copy link
Contributor
@DavidLP DavidLP commented May 13, 2017

Description:

The used python library spotipy version 2.4.4 from https://github.com/happyleavesaoc/spotipy/archive/544614f4b1d508201d363e84e871f86c90aa26b2.zip returns for spotipy.Spotify(auth=...).devices() either a python dict or None. The None case was not treated leading to a crash.

Related issue (if applicable): fixes #7384

Example entry for configuration.yaml (if applicable):

   media_player:
    - platform: spotify
      client_id: XXXXXXXXXXXXXXXXXXXXXXXXXXXX
      client_secret: XXXXXXXXXXXXXXXXXXXXXXXXX

Checklist:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Local installation works after bug fix

@mention-bot
Copy link

@DavidLP, thanks for your PR! By analyzing the history of the files in this pull request, we identified @happyleavesaoc, @fabaff and @abmantis to be potential reviewers.

@abmantis
Copy link
Member

You say that the None case was not treated, but you added exception handling. Was the exception that was not catch before?

@DavidLP
Copy link
Contributor Author
DavidLP commented May 14, 2017

The exception occured when using None like a dict. None does not have a get method.

@abmantis
Copy link
Member

Isn't it better to just do a if self._player.devices() is None: .... instead of the try?

@DavidLP
Copy link
Contributor Author
DavidLP commented May 15, 2017

It is not guaranteed that repeated calls to self._player.devices() will return the same. I actually observed that the first call returned a dict, but the second one None. One could of cause save the self._player.devices() return value in a variable, but I think that the exception handling here is fine.

@abmantis
Copy link
Member

Ok. Looks good to me then!

@bieniu bieniu mentioned this pull request May 16, 2017
Closed
@DavidLP
Copy link
Contributor Author
DavidLP commented May 16, 2017

At another point the same unsafe API call is done, that I have to fix first. So please do not merge yet.

@DavidLP DavidLP changed the title Quickfix Bug #7384 [WIP] Quickfix Bug #7384 May 16, 2017
@DavidLP DavidLP changed the title [WIP] Quickfix Bug #7384 Quickfix Bug #7384 May 25, 2017
@DavidLP
Copy link
Contributor Author
DavidLP commented May 25, 2017

I do not have any issues after this fix. Without this fix spotify plugin cannot be used right now. Thus I recomment merging and releasing in next bugfix releae.

@fanaticDavid fanaticDavid mentioned this pull request May 30, 2017
Closed
@balloob balloob merged commit 2b70b18 into home-assistant:dev Jun 2, 2017
@balloob balloob mentioned this pull request Jun 2, 2017
Merged
@home-assistant home-assistant locked and limited conversation to collaborators Sep 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@abmantis abmantis abmantis approved these changes

Assignees
No one assigned
Labels
cla-signed integration: spotify
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Spotify media_player errors when all players are off/disconnected
5 participants
@DavidLP @mention-bot @abmantis @balloob @homeassistant
0