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

Add websocket calls to shopping-list #18392

Merged
merged 6 commits into from Nov 20, 2018

Conversation

Projects
None yet
5 participants
@iantrich
Contributor

iantrich commented Nov 12, 2018

Plan to deprecate API calls once shopping-list panel is removed from UI and replaced fully by Lovelace card

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
Add websocket calls to shopping-list
Plan to deprecate API calls once shopping-list panel is removed from UI and replaced fully by Lovelace card
Show resolved Hide resolved tests/components/test_shopping_list.py Outdated
Show resolved Hide resolved tests/components/test_shopping_list.py Outdated
Show resolved Hide resolved tests/components/test_shopping_list.py Outdated
Show resolved Hide resolved tests/components/test_shopping_list.py Outdated
Show resolved Hide resolved tests/components/test_shopping_list.py Outdated
Show resolved Hide resolved homeassistant/components/shopping_list.py Outdated
Show resolved Hide resolved homeassistant/components/shopping_list.py Outdated
Show resolved Hide resolved homeassistant/components/shopping_list.py Outdated
Show resolved Hide resolved homeassistant/components/shopping_list.py Outdated
Show resolved Hide resolved homeassistant/components/shopping_list.py Outdated
@iantrich

This comment has been minimized.

Contributor

iantrich commented Nov 12, 2018

I'm new to python dev. Is there a a good way I could have caught these ci-bot issues before I made my PR?

iantrich added some commits Nov 12, 2018

@bramkragten

This comment has been minimized.

Member

bramkragten commented Nov 12, 2018

@@ -256,3 +303,46 @@ def post(self, request):
hass.data[DOMAIN].async_clear_completed()
hass.bus.async_fire(EVENT)
return self.json_message('Cleared completed items.')
@websocket_api.async_response

This comment has been minimized.

@balloob

balloob Nov 18, 2018

Member

There is no reason for this one to be async, it's not doing any async work.

@websocket_api.async_response
async def websocket_handle_add(hass, connection, msg):

This comment has been minimized.

@balloob

balloob Nov 18, 2018

Member

No need to be async

data = {}
if 'name' in msg:
data = {'name': msg['name']}
else:

This comment has been minimized.

@balloob

balloob Nov 18, 2018

Member

Why else, can't both be there?

I think that we should do this:

msg_id = msg.pop('id')
msg.pop('type')

data = msg
hass.bus.async_fire(EVENT)
connection.send_message(websocket_api.result_message(
msg['id'], item))
except KeyError:

This comment has been minimized.

@balloob

balloob Nov 18, 2018

Member

We should wrap key error only around the part that can raise the error.

msg['id'], 'shopping_list_update_failed', 'Item not found'))
@websocket_api.async_response

This comment has been minimized.

@balloob

balloob Nov 18, 2018

Member

No need to be async either.

async def websocket_handle_clear(hass, connection, msg):
"""Handle clearing shopping_list items."""
hass.data[DOMAIN].async_clear_completed()
hass.bus.async_fire(EVENT)

This comment has been minimized.

@balloob

balloob Nov 18, 2018

Member

Isn't it weird that we're duplicating functionality between http and websocket. This should probably be done insidethe async_clear_completed method.

hass.data[DOMAIN].async_clear_completed()
hass.bus.async_fire(EVENT)
connection.send_message(websocket_api.result_message(
msg['id'], 'Cleared completed items.'))

This comment has been minimized.

@balloob

balloob Nov 18, 2018

Member

No need to return a response. Just websocket_api.result_message(msg['id'])

@balloob

This comment has been minimized.

Member

balloob commented Nov 18, 2018

@iantrich, wouldn't the MVP approach be if just 1 command was per PR? 😉

@@ -36,6 +37,35 @@
vol.Required(ATTR_NAME): vol.Any(None, cv.string)
})
WS_TYPE_SHOPPING_LIST_GET_ITEMS = 'shopping_list/items/get'

This comment has been minimized.

@balloob

balloob Nov 18, 2018

Member

We've been using list instead of get in other parts of the code.

@iantrich

This comment has been minimized.

Contributor

iantrich commented Nov 18, 2018

@balloob yeah I can do that and make this a MVP for a single call to make sure I have the best structure before proceeding. Good idea. Thanks.

@balloob

This comment has been minimized.

Member

balloob commented Nov 19, 2018

It's not a new idea 😝

iantrich added some commits Nov 20, 2018

@balloob balloob merged commit 44b33d4 into home-assistant:dev Nov 20, 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.02%) to 93.028%
Details

@wafflebot wafflebot bot removed the in progress label Nov 20, 2018

mxworm added a commit to mxworm/home-assistant that referenced this pull request Nov 20, 2018

Merge branch 'dev' of https://github.com/home-assistant/home-assistant
…into dev

* 'dev' of https://github.com/home-assistant/home-assistant:
  Improve available for Mill heater (home-assistant#18597)
  Z-Wave Lock Config Entry Support (home-assistant#18209)
  Add support for Daikin BRP069B41 (home-assistant#18564)
  Hass.io config check (home-assistant#18576)
  Revert changes that broke UI (home-assistant#18495)
  Add websocket calls to shopping-list (home-assistant#18392)
  Bump Python-Nest to 4.0.5 (home-assistant#18580)
  Adds light switch platform (home-assistant#18562)
  Added unique id to all Wink devices. (home-assistant#18589)

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

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