Skip to content

Update group reload string#7938

Merged
bramkragten merged 3 commits intohome-assistant:devfrom
cdce8p:update-reload-string
Dec 29, 2020
Merged

Update group reload string#7938
bramkragten merged 3 commits intohome-assistant:devfrom
cdce8p:update-reload-string

Conversation

@cdce8p
Copy link
Copy Markdown
Member

@cdce8p cdce8p commented Dec 10, 2020

Proposed change

Update string for group reload service in server control. Especially if this entry is the only one mentioning notify services, a user could assume that it would reload the notify component. See below. Removing the comma makes it clear that it's the group notify services which will be reloaded. It will also be more consistent with other reload strings, eg. for rest

"rest": "Reload rest entities and notify services",

Screen Shot 2020-12-10 at 13 05 02

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

--

Additional information

--

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

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

"reload": "Reload {domain}",
"core": "Reload location & customizations",
"group": "Reload groups, group entities, and notify services",
"group": "Reload groups, group entities and notify services",
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.

Suggested change
"group": "Reload groups, group entities and notify services",
"group": "Reload groups, group entities, and group notify services",

Wouldn't that make it even clearer?

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.

Works for me :)

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.

Then I would propose we do the same for "rest" to be consistent.

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.

If I didn't miss any, there are three other strings that mention notify services.
On one hand telegram and smtp are probably fine:

"telegram": "Reload telegram notify services",
"smtp": "Reload SMTP notify services",

rest on the other could be changed:

"rest": "Reload rest entities and notify services",

Although to be fair I don't think it's as bad as with group since it IMO clearly applies to rest.

cdce8p and others added 2 commits December 16, 2020 23:45
Co-authored-by: Philip Allgaier <philip.allgaier@gmx.de>
@bramkragten bramkragten merged commit 50c5c15 into home-assistant:dev Dec 29, 2020
@bramkragten bramkragten mentioned this pull request Dec 29, 2020
@cdce8p cdce8p deleted the update-reload-string branch December 30, 2020 01:14
@github-actions github-actions bot locked and limited conversation to collaborators Dec 31, 2020
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.

4 participants