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.

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

Added Twitch v5 support to the twitch platform #16428

Merged
merged 15 commits into from Sep 6, 2018

Conversation

@ioangogo
Contributor

ioangogo commented Sep 4, 2018

THIS IS A BREAKING CHANGE

Description:

This is an update that changes the library use for the twitch sensor, this is a breaking change due to api Changes on twitch's side where all api users require a client_id. I have also made it set the sensor icon to the channels logo when they are not live, as it looks nice.

Middle is live, the others are offline

Middle is live, the others are offline

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6201

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: twitch
    clientid: "clientid"
    channels:
    - user1
    - user2

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example). kind of, i set up the client in the setup and pass the client through to the entities, you can ask me to change this
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@ioangogo ioangogo referenced this pull request Sep 4, 2018

Merged

Update sensor.twitch.markdown #6201

2 of 2 tasks complete

@ioangogo ioangogo changed the title from [wip]added Twitch v5 support to the twitch component, This is a breaking change to added Twitch v5 support to the twitch component, This is a breaking change Sep 4, 2018

@ioangogo ioangogo changed the title from added Twitch v5 support to the twitch component, This is a breaking change to added Twitch v5 support to the twitch component Sep 5, 2018

@fabaff fabaff referenced this pull request Sep 5, 2018

Closed

Timeouts #44

@fabaff

tests/testing_config/.storage/core.entity_registry should be part of this PR.

@fabaff fabaff changed the title from added Twitch v5 support to the twitch component to Added Twitch v5 support to the twitch platform Sep 5, 2018

@ioangogo

This comment has been minimized.

Show comment
Hide comment
@ioangogo

ioangogo Sep 5, 2018

Contributor

tests/testing_config/.storage/core.entity_registry

i assume this shoulent be in there, I have removed it from my branch

Contributor

ioangogo commented Sep 5, 2018

tests/testing_config/.storage/core.entity_registry

i assume this shoulent be in there, I have removed it from my branch

ioangogo and others added some commits Sep 5, 2018

@fabaff

fabaff approved these changes Sep 6, 2018

@fabaff fabaff merged commit ce06229 into home-assistant:dev Sep 6, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 93.762%
Details

@wafflebot wafflebot bot removed the in progress label Sep 6, 2018

@ioangogo ioangogo deleted the ioangogo:twitchv5 branch Sep 6, 2018

client.ingests.get_server_list()
except HTTPError:
_LOGGER.error("Client ID is not valid")
return False

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 6, 2018

Member

Nothing is checking this return value.

@MartinHjelmare

MartinHjelmare Sep 6, 2018

Member

Nothing is checking this return value.

This comment has been minimized.

@ioangogo

ioangogo Sep 7, 2018

Contributor

the orignial code did check this by checking for a 400 error, for the endpoint im using 400 error should only be caused by a missing client id

        if http_error.response.status_code == 400:
            _LOGGER.error("Test API call failed, Check your client_id")
        else:
            _LOGGER.error("Test API call failed")
        return False```
@ioangogo

ioangogo Sep 7, 2018

Contributor

the orignial code did check this by checking for a 400 error, for the endpoint im using 400 error should only be caused by a missing client id

        if http_error.response.status_code == 400:
            _LOGGER.error("Test API call failed, Check your client_id")
        else:
            _LOGGER.error("Test API call failed")
        return False```

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 7, 2018

Member

No, nothing is checking this return value. Don't return False, just return.

@MartinHjelmare

MartinHjelmare Sep 7, 2018

Member

No, nothing is checking this return value. Don't return False, just return.

@ioangogo ioangogo restored the ioangogo:twitchv5 branch Sep 7, 2018

@balloob balloob referenced this pull request Sep 17, 2018

Merged

0.78.0 #16666

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment