-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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 styled formatting option to Signal Messenger integration - Bump pysignalclirestapi to 0.3.24 #117148
Add styled formatting option to Signal Messenger integration - Bump pysignalclirestapi to 0.3.24 #117148
Conversation
Hey there @bbernhard, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
Are you sure that those changes are enough? I think for the text_mode parameter to work, there are also changes in the signal integration needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mainfest, requirements_all.txt and requirements_test_all.txt are changed. Looks ok.
@bbernhard thanks for the heads up, I ran the local tests but I could not double check with a real instance. |
@bbernhard I added the parameter in notify.py. I followed the example of other additional parameters, though I am wondering if it could be moved to the main data structure: action:
service: notify.NOTIFIER_NAME
data:
message: "Test **message**"
data:
text_mode: "styled" to action:
service: notify.NOTIFIER_NAME
data:
message: "Test **message**"
text_mode: "styled" |
Thanks for the PR!
good question. Personally I do not have a preference - both is fine for me. But not sure if the Home Assistant core team has some guidelines for that. |
@bbernhard Currently the main "breaking" change would be for url-defined attachments:
action:
service: notify.NOTIFIER_NAME
data:
message: "Person detected on Front Camera!"
data:
verify_ssl: false
urls:
- "http://homeassistant.local/api/frigate/notifications/<event-id>/thumbnail.jpg"
action:
service: notify.NOTIFIER_NAME
data:
message: "Person detected on Front Camera!"
data:
urls:
verify_ssl: false
urls:
- "http://homeassistant.local/api/frigate/notifications/<event-id>/thumbnail.jpg" If that is ok from your side, I will add some additional tests for text_mode and mark for review.
data:
attachments:
filenames:
- "/config/www/surveillance_camera.jpg"
data:
attachements:
verify_ssl: false
urls:
- "http://homeassistant.local/api/frigate/notifications/<event-id>/thumbnail.jpg" |
Thanks a lot for the update @r-xyz! I am not sure if I am qualified to answer that, as I do not know how the Home Assistant core team sees breaking changes. Personally, from the perspective of a simple Home Assistant user, I am not a big fan of breaking changes, as they require me to actively change something to get the existing behavior working again. From the perspective of a maintainer of other open source projects, I am not a big fan either, as breaking changes usually result in quite some follow up issues (e.g "Why doesn't this work anymore?"). But I can see that they are sometimes necessary to clean up technical debt. I know that in the early days of Home Assistant, breaking changes occurred quite often and one had to fix stuff manually after almost every release (at least that's how it felt). Due to shortage of free time however, I am not following the Home Assistant development in detail anymore, so I do not know what their stance on breaking changes is. Mabye someone from the Core team can chime in here. Although personally I am not in favor of breaking changes, I am totally fine with it in case the Core team is fine with it :) |
Signed-off-by: r-xyz <100710244+r-xyz@users.noreply.github.com>
Signed-off-by: r-xyz <100710244+r-xyz@users.noreply.github.com>
Hi @bbernhard, |
Many thanks for working around the breaking change - very much appreciated! From my side the extension looks good! ✔️ Thanks a lot for for working on this - this will make quite some Home Assistant users happy :) |
Added additional tests and docs. |
has-tests and new-feature tags were be added automatically since PR was initially opened without. |
@frenck Please let me know in case additional changes are needed. |
Improved exception handling, codecov is now also fully passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there @r-xyz 👋
Thanks for the PR! Left some suggestions to let the schema validation handle all these default and validation magic, removing the need for the get_text_mode
method.
../Frenck
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Requested changes were implemented. One test needed to be modified, too, since invalid text_mode attribute now gives error instead of defaulting to "normal" with warning (previous behaviour). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @r-xyz 👍
../Frenck
pysignalclirestapi v0.3.24 enables "text_mode" for the Signal Messenger integration.
Changelog: bbernhard/pysignalclirestapi@0.3.23...0.3.24
Breaking change
Proposed change
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: