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

Removed assumptions about provided upnp data #17604

Merged
merged 3 commits into from Nov 1, 2018

Conversation

Projects
None yet
6 participants
@WoLpH
Contributor

WoLpH commented Oct 18, 2018

Description:

Some devices don't provide all upnp data but the code expects it to be there.

Example stacktrace:

Traceback (most recent call last):
  File "home-assistant/homeassistant/components/discovery.py", line 145, in new_service_found
    data=info
  File "home-assistant/homeassistant/data_entry_flow.py", line 64, in async_init
    return await self._async_handle_step(flow, flow.init_step, data)
  File "home-assistant/homeassistant/data_entry_flow.py", line 98, in _async_handle_step
    result = await getattr(flow, method)(user_input)  # type: Dict
  File "home-assistant/homeassistant/components/upnp/config_flow.py", line 71, in async_step_discovery
    '{} ({})'.format(discovery_info['host'], discovery_info['name'])
KeyError: 'name'

Example entry for configuration.yaml (if applicable):

Not applicable

Checklist:

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

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

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

  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

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

This comment has been minimized.

Member

dgomes commented Oct 19, 2018

# add name if available
if discovery_info.get('name'):
print('test', discovery_info)

This comment has been minimized.

@dgomes

dgomes Oct 19, 2018

Member

No print()

@dgomes

please cleanup

@WoLpH WoLpH force-pushed the WoLpH:upnp-make-no-assumptions branch from f8777ea to e101a83 Oct 21, 2018

@WoLpH

This comment has been minimized.

Contributor

WoLpH commented Oct 21, 2018

Sorry about that, after I made the pull request I noticed another error and was in the process of fixing it.

I've cleaned up the fix and squashed the commits but I'm not seeing it here yet, I'm guessing Github is caching something so if it doesn't clear up soon I'll create a new pull request :)
The actual commit is here: WoLpH@e101a83

@WoLpH

This comment has been minimized.

Contributor

WoLpH commented Oct 22, 2018

It looks like Github has resolved most of the issues. Can you please check again @dgomes?

@dgomes

This comment has been minimized.

Member

dgomes commented Oct 22, 2018

I would like a review first from @SteveEdson as current code owner.

@dgomes

dgomes approved these changes Oct 22, 2018

@SteveEdson

This comment has been minimized.

Contributor

SteveEdson commented Oct 22, 2018

Are you sure you mean me?...

@dgomes

This comment has been minimized.

Member

dgomes commented Oct 22, 2018

Ups ... wrong Steve

@StevenLooman

@StevenLooman

This comment has been minimized.

Contributor

StevenLooman commented Oct 23, 2018

Looks good to me!

@dgomes dgomes added Ready for review and removed in progress labels Oct 23, 2018

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 30, 2018

Please solve the merge conflict too.

@WoLpH

This comment has been minimized.

Contributor

WoLpH commented Oct 31, 2018

@MartinHjelmare I've merged the changes from the dev branch :)

@MartinHjelmare

Good! Can be merged when build passes.

@WoLpH

This comment has been minimized.

Contributor

WoLpH commented Oct 31, 2018

Great!

Those builds definitely need a speedup btw :P

@WoLpH

This comment has been minimized.

Contributor

WoLpH commented Nov 1, 2018

It seems the travis bot got stuck somehow... the build did succeed: https://travis-ci.org/home-assistant/home-assistant/builds/448924071

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Nov 1, 2018

I've restarted the shortest job to see if we can trigger a green light. We like those ✔️.

@MartinHjelmare MartinHjelmare merged commit 82edea6 into home-assistant:dev Nov 1, 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 increased (+0.005%) to 93.094%
Details

@wafflebot wafflebot bot removed the Ready for review label Nov 1, 2018

@StevenLooman

This comment has been minimized.

Contributor

StevenLooman commented Nov 2, 2018

Great! @WoLpH Thank you for the changes.

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

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