Skip to content

Add huawei_lte notify component#19544

Merged
MartinHjelmare merged 2 commits into
home-assistant:devfrom
scop:huawei-lte-notify
Feb 1, 2019
Merged

Add huawei_lte notify component#19544
MartinHjelmare merged 2 commits into
home-assistant:devfrom
scop:huawei-lte-notify

Conversation

@scop
Copy link
Copy Markdown
Member

@scop scop commented Dec 23, 2018

Description:

Add notify component using Huawei LTE router SMS.

Related issue (if applicable): https://community.home-assistant.io/t/adding-notification-to-huawei-lte-router/70950

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

Example entry for configuration.yaml (if applicable):

notify:
  - platform: huawei_lte
    target: "+15105550123"

Checklist:

  • The code change is tested and works locally. Caveat: tested without an SMS enabled plan, so verified only that the API calls succeed and fail when/like they should, and sent messages end up in the router's SMS outbox.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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).
  • 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.

@andyinno
Copy link
Copy Markdown

andyinno commented Dec 28, 2018

Changing

from ..huawei_lte import DATA_KEY

to

from homeassistant.components.huawei_lte import DATA_KEY

Allowed me to test it as custom component.
I tested the sending of the sms 4 times and 2 times it was successful. The other two times I reached line 60 and the error message appeared.

@scop
Copy link
Copy Markdown
Member Author

scop commented Jan 16, 2019

Anything I can do to get this moving forward? I don't think there's anything to be done for send failures besides logging them here, if they're not about some syntax errors caused by the implementation (which I think they aren't, but can't obviously tell for sure without seeing the error messages).

@andyinno
Copy link
Copy Markdown

andyinno commented Jan 16, 2019 via email

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good! Just a small comment.


PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Optional(CONF_URL): cv.url,
vol.Required(ATTR_TARGET): vol.All(cv.ensure_list, [cv.string]),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should use CONF_ constants for config keys

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Changed to CONF_RECIPIENT as that seems to be used for this purpose in other notify component configs. But note that the similar netgear_lte and tplink_lte components use ATTR_TARGET, that's where I initially picked it up from. Maybe it should somehow be fixed in them too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be good to fix that, yes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's target in the notify service itself so I'm not sure why we would use recipient in the platforms?

https://www.home-assistant.io/components/notify/#service

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For consistency within platforms? I have no strong opinions either way, but the majority of platforms already use recipient (clickatell, clicksend, clicksend_tts, mailgun, sendgrid, smtp, xmpp, yessssms, and now huawei_lte).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm aiming for consistency, period. Switching nine platforms over now might seem like a lot of work, but better to do it before it's twentynine ...

I also have no particular preference for either word but for usability alone, I think it should be the same everywhere.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Target is easier to spell. 😄

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Recipient is more descriptive :)

@MartinHjelmare MartinHjelmare merged commit 3f997ae into home-assistant:dev Feb 1, 2019
@ghost ghost removed the in progress label Feb 1, 2019
@scop scop deleted the huawei-lte-notify branch February 1, 2019 19:40
@balloob balloob mentioned this pull request Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants