Skip to content

Add action callbacks to simplepush#80744

Closed
tymm wants to merge 3 commits into
home-assistant:devfrom
tymm:update-simplepush-component
Closed

Add action callbacks to simplepush#80744
tymm wants to merge 3 commits into
home-assistant:devfrom
tymm:update-simplepush-component

Conversation

@tymm
Copy link
Copy Markdown
Contributor

@tymm tymm commented Oct 21, 2022

Breaking change

Proposed change

This PR adds action callback functionality for the Simplepush component.
It also aims to use Home Assistant's shared aiohttp client session in order to improve performance.

I will update the documentation (https://www.home-assistant.io/integrations/simplepush/), after this PR gets merged. If preferred I can also do it beforehand.

The new functionality can be tested with the following automation:

alias: Test
description: >-
  Send an action and if triggered within 10 seconds, log success message to the
  logbook
trigger: []
condition: []
action:
  - service: notify.simplepush
    data:
      message: test
      data:
        feedback_action_timeout: 10
        actions:
          - action: "yes"
            id: 123
  - wait_for_trigger:
      - platform: event
        event_type: simplepush_action_triggered_event
        event_data:
          id: 123
  - service: logbook.log
    data:
      name: Test
      message: Success
mode: restart

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)
  • Deprecation (breaking change to happen in the future)
  • 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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link
Copy Markdown
Contributor

Hey there @engrbm87, mind taking a look at this pull request as it has been labeled with an integration (simplepush) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of simplepush can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant unassign simplepush Removes the current integration label and assignees on the issue, add the integration domain after the command.

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.

I don't think the way the callback is implemented in the stack is good. Polling for a response like that is not good.

actions=actions,
attachments=attachments,
event=event,
feedback_callback=feedback_action_callback,
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.

The callback will potentially make a request every second for one minute. That doesn't seem prudent.

https://github.com/simplepush/simplepush-python/blob/52f4cf2b072f42a1dde94698f5049ce122dddfb8/simplepush/__init__.py#L258-L286

n is never increased in the loop either.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

While it is certainly not prudent, I think it is a good enough first step considering that I will probably be the only user of this feature for a while anyway and an alternative solution would require a lot of work.

However, I would of course understand if Home Assistant doesn't want to subscribe to the move-fast-break-things philosophy as much as I have to.

Could there be a compromise for now where we set a fixed timeout?

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're not some playground. We're real people spending real time maintaining Home Assistant and reviewing PRs. If you are making something that is only used by yourself and it's full of hacks… it doesn't belong in HA. I suggest to make it available as a custom component that people can install to HACS. There are 900 PRs open across Home Assistant, all intended for a bigger audience.

title=title,
message=message,
event=event,
self.hass.async_create_task(
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.

If we create a task the exceptions won't be caught here. We should await here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, this is a bit tricky.
If I don't create a task and use await instead, then a wait_for_trigger (see the example I provided in the description) will never be triggered.
And the possibility to use wait_for_trigger is crucial for this feature.

Do you have an idea on how to resolve this?

Comment thread homeassistant/components/simplepush/notify.py
except (ValueError, TypeError):
feedback_action_timeout = 60

attachments_data = data.get(ATTR_ATTACHMENTS)
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.

Please limit the PR to one thing. Don't both add callbacks and attachments.

Copy link
Copy Markdown
Contributor Author

@tymm tymm Oct 26, 2022

Choose a reason for hiding this comment

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

Okay, I removed attachments from this PR and create a separate PR for them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I created #81033 for attachments.

@tymm tymm changed the title Update simplepush component Add action callbacks to simplepush Oct 26, 2022
@tymm tymm mentioned this pull request Oct 26, 2022
19 tasks
@balloob balloob closed this Oct 27, 2022
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 28, 2022
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.

3 participants