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.

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

Update vizio app_id state attribute to show app config dict instead of app name #32727

Merged
merged 6 commits into from Mar 25, 2020

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Mar 12, 2020

Proposed change

This PR updates the app_id state attribute to show the config properties needed to identify an app. It is possible for the user to manually configure the app for detection in HA (when pyvizio can’t detect the app name natively) using the configuration provided by the Vizio API, but this previously required users to install and run pyvizio locally whereas now users can get this information from Home Assistant directly.

I'm also reducing the number of calls to the Vizio API from 2 to 1 by simplifying the logic to retrieve the app name

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@project-bot project-bot bot added this to Needs review in Dev Mar 12, 2020
@project-bot project-bot bot moved this from Needs review to By Code Owner in Dev Mar 12, 2020
@raman325 raman325 changed the title Add device_state_attributes to vizio to show app_config when app name can't be determined Update vizio app_id to show id dict Mar 13, 2020
@raman325 raman325 changed the title Update vizio app_id to show id dict Update vizio app_id to show app config dict Mar 13, 2020
@raman325 raman325 changed the title Update vizio app_id to show app config dict Update vizio app_id state attribute to show app config dict instead of app name Mar 13, 2020
… generation. squashed from multiple commits to make a rebase on dev easier
@raman325
Copy link
Contributor Author

Per @MartinHjelmare 's feedback on #33051 I will be submitting a separate PR to remove the conditional logic out of my test helpers. So when reviewing this PR please ignore that issue for now, as it will require significant changes and will be easier to submit and review separately.

@raman325 raman325 mentioned this pull request Mar 24, 2020
20 tasks
Dev automation moved this from By Code Owner to Reviewer approved Mar 25, 2020
@balloob balloob merged commit d5f4dfd into home-assistant:dev Mar 25, 2020
Dev automation moved this from Reviewer approved to Done Mar 25, 2020
@raman325 raman325 deleted the vizio_app_config_attribute branch March 25, 2020 16:44
@lock lock bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants