Skip to content

Conversation

@emontnemery
Copy link
Contributor

Proposed change

Improve trigger descriptions as discussed with @jlpouffier

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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:
  • Link to developer documentation pull request:
  • Link to frontend pull request:

Checklist

  • I understand the code I am submitting and can explain how it works.
  • 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
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.
  • Any generated code has been carefully reviewed for correctness and compliance with project standards.

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.

To help with the load of incoming pull requests:

},
"title": "Assist satellite",
"triggers": {
"idle": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The keys for assist_satellite are not consistent with other domains

},
"title": "Lawn mower",
"triggers": {
"docked": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we change this to "returned_to_dock", or is "docked" good enough?

Copy link
Member

Choose a reason for hiding this comment

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

Docked is good.
You explained it too me, we do not enforce the temporary state "Returning to dock". So it's for both. IT's good 👍🏻

"name": "When a lawn mower has docked"
"name": "Lawn mower returned to dock"
},
"errored": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we change this to "encounted_error", or is "errored" good enough?

},
"title": "Vacuum",
"triggers": {
"docked": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as for lawn mower

"name": "When a vacuum cleaner has docked"
"name": "Vacuum returned to dock"
},
"errored": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as for lawn mower

"triggers": {
"armed": {
"description": "Triggers when an alarm is armed.",
"description": "Triggers when one or several alarms are armed, regardless of the mode.",
Copy link
Contributor Author

@emontnemery emontnemery Dec 1, 2025

Choose a reason for hiding this comment

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

I discussed this a bit with @jlpouffier.
He first suggested "Triggers when one or several alarms are armed, regardless of the mode.", then I suggested to drop the "get", and in the PR I changed to "are".
Edit: Changed back to "get"

Copy link

Choose a reason for hiding this comment

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

Why not replace 'several' with 'more' to simplify the wording?

Copy link
Member

Choose a reason for hiding this comment

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

CleanShot 2025-12-01 at 21 12 45

OK with several >>> more.

Not a strong push, i'm also perfectly fine with several for that particular PR.
We've been struggling with wording for so long that I'm more willing to push something out the finish line now :D

This comment was marked as off-topic.

Copy link
Contributor

@jbouwh jbouwh left a comment

Choose a reason for hiding this comment

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

LGTM,
Thanks @emontnemery 👍

@emontnemery emontnemery added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Dec 1, 2025
@emontnemery
Copy link
Contributor Author

I want @jlpouffier to review before this is merged

@emontnemery
Copy link
Contributor Author

I hid the bot review comments because I don't think they were helpful

@MartinHjelmare MartinHjelmare marked this pull request as draft December 1, 2025 17:15
@emontnemery
Copy link
Contributor Author

@MartinHjelmare there have been a lot of discussions about the wording.
I think the pattern "Triggers when one or several fans turn on" is good, I'm not sure if we have considered that variant. @jlpouffier ?

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Dec 1, 2025

Grammar isn't really something to discuss. It's either correct or wrong. What to discuss is what tense to use for the description.

@emontnemery
Copy link
Contributor Author

OK, let's not discuss grammar here then. Closing for now.

@emontnemery emontnemery closed this Dec 1, 2025
@emontnemery emontnemery reopened this Dec 1, 2025
@emontnemery emontnemery requested a review from Copilot December 1, 2025 19:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.


You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@jlpouffier
Copy link
Member

jlpouffier commented Dec 1, 2025

I'm ok with @MartinHjelmare proposal
After + Past tense instead of When + Present
It is both

  • Grammatically correct
  • Aligned with the technical names of the triggers.

Triggers after a light turned on. Perfectly understandable.

EDIT: on hold, discussing with Martin .. Not sure I fully understand the nuances

OK, discussing with @MartinHjelmare

It's actually After + Present that is correct.
Triggers after one or many lights turn on

I'm ok with that, i'll review now with that in mind

@jlpouffier
Copy link
Member

@emontnemery I made lots of comments, but only the first one is actually something I'd like to see changed ☺️

I made a proposal alighed with other triggers

@emontnemery emontnemery marked this pull request as ready for review December 2, 2025 07:01
@MartinHjelmare MartinHjelmare removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Dec 2, 2025
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks, @emontnemery 👍

../Frenck

                       

Blogging my personal ramblings at frenck.dev

@frenck frenck merged commit 393be71 into dev Dec 2, 2025
66 checks passed
@frenck frenck deleted the improve_trigger_descriptions branch December 2, 2025 10:08
@frenck frenck added this to the 2025.12.0 milestone Dec 2, 2025
frenck pushed a commit that referenced this pull request Dec 2, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Dec 3, 2025
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.

7 participants