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

Dominos no order fix #10935

Merged
merged 6 commits into from
Dec 4, 2017
Merged

Conversation

craigjmidwinter
Copy link
Contributor

Description:

Couple of small fixes for dominos component

Related issue (if applicable): fixes #10929

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

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

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New 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:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

try:
self.closest_store = self.address.closest_store()
except StoreException:
self.closest_store = Falsee

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undefined name 'Falsee'

@balloob balloob added this to the 0.59.1 milestone Dec 4, 2017
for order_info in conf.get(ATTR_ORDERS):
order = DominosOrder(order_info, dominos)
entities.append(order)
if conf.get(ATTR_ORDERS) is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather default to an empty list in the config schema and add a check below if entities is empty before adding them.

@@ -58,7 +58,8 @@
vol.Required(ATTR_PHONE): cv.string,
vol.Required(ATTR_ADDRESS): cv.string,
vol.Optional(ATTR_SHOW_MENU): cv.boolean,
vol.Optional(ATTR_ORDERS): vol.All(cv.ensure_list, [_ORDERS_SCHEMA]),
vol.Optional(ATTR_ORDERS, default={}): vol.All(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default to [].

menu = self.closest_store.get_menu()
product_entries = []
return []
else:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else is not necessary because you return inside the if-statement.

@balloob balloob merged commit bd6a17a into home-assistant:dev Dec 4, 2017
balloob pushed a commit that referenced this pull request Dec 4, 2017
* check for none

* fix error from no store being set

* typo

* Lint

* fix default as per notes. Lint fix and make closest store None not False

* update default
@balloob balloob mentioned this pull request Dec 4, 2017
@Coolie1101
Copy link

Still the same after upgrading to 0.59.1

@craigjmidwinter
Copy link
Contributor Author

@Coolie1101 What does the error log look like now? Still a json.decoder.JSONDecodeError?

@Coolie1101
Copy link

No error's in log for Dominos, but Custom Panel is still blank.

@craigjmidwinter
Copy link
Contributor Author

craigjmidwinter commented Dec 5, 2017

@Coolie1101 - would you mind trying to put this in your custom_components and see if it corrects the issue for you?

https://gist.github.com/wardcraigj/1b215c680b0179d50c0f8f54c5e29510

@Coolie1101
Copy link

Does the config stay the same?

@Coolie1101
Copy link

Incidentally, that was already in my custom_components/sensor directory.

Should I remove it?

@craigjmidwinter
Copy link
Contributor Author

Yep, you can take it out of custom_components/sensor and put the one I linked to into custom_components/

The version I linked to is the version that will be in 0.58.2 so once that's out you can pull that version out of custom_components all together

@Coolie1101
Copy link

Coolie1101 commented Dec 6, 2017

So, I upgraded to 0.59.2, and it looks like something is working, aa I get the following Warning:

Log Details (WARNING)
Wed Dec 06 2017 07:58:37 GMT-0500 (Eastern Standard Time)

Cannot get menu. Store may be closed

@craigjmidwinter
Copy link
Contributor Author

Yeah, that looks right, currently the store needs to be open in order to pull the menu. Try when the store opens and hopefully it works

@Coolie1101
Copy link

Coolie1101 commented Dec 7, 2017

@wardcraigj:
Sorry for the delayed response, been on the road all day.

It's now 8:40 PM local time, the Dominos card displays as below, the custom panel is still blank, and my local store closes at 2:00AM

image

FYI, I'm not complaining in any way, just providing info in an effort to help troubleshoot potential issues, as this feature would be a great addition to automating, it's not critical that is work.

Thanks for all your efforts.

Let me know if there is anything else I should check.

@home-assistant home-assistant locked and limited conversation to collaborators Dec 7, 2017
@balloob
Copy link
Member

balloob commented Dec 7, 2017

No support in PRs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dominos Config Error
7 participants