Skip to content

growatt_server: use human-readable labels in exception messages#166024

Open
johanzander wants to merge 12 commits intohome-assistant:devfrom
johanzander:fix/growatt-gold-action-wording
Open

growatt_server: use human-readable labels in exception messages#166024
johanzander wants to merge 12 commits intohome-assistant:devfrom
johanzander:fix/growatt-gold-action-wording

Conversation

@johanzander
Copy link
Contributor

@johanzander johanzander commented Mar 19, 2026

Proposed change

Follow-up to #165314, fixing wording issues in strings.json for the Growatt Server integration.

  • "service" → "action" wording: The device_not_configured exception message incorrectly said "services" — Home Assistant renamed these to "actions".
  • Snake_case identifiers in user-visible strings: Exception messages exposed internal Python parameter names (e.g. charge_power must be between 0 and 100). Replaced with proper English labels wrapped in single quotes ('Charge power', 'Charge stop SOC', 'Discharge power', 'Discharge stop SOC', 'Segment ID', 'Start time', 'End time', 'Period N start', 'Period N end').
  • Action descriptions use third-person singular: All action descriptions now use Reads…, Writes…, Updates… to match HA standards.
  • Neutral device_id field description for all SPH actions: All four SPH actions now reference the neutral description from read_time_segments instead of action-specific wording.

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

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

HA renamed "services" to "actions" in the UI. Update the remaining
exception message in strings.json that still used the old term.

Fixes review comment by @NoRi2909 on home-assistant#165927.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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

This PR updates the Growatt Server integration’s exception translation string to use Home Assistant’s current “actions” terminology instead of the deprecated “services” wording.

Changes:

  • Updates the device_not_configured exception message in strings.json from “services” to “actions”.

@johanzander johanzander marked this pull request as draft March 19, 2026 20:37
…able labels in exception messages

Use proper English in translated exception messages instead of internal
Python parameter names. Charge power, discharge power, SOC limits and
time-format errors now show user-friendly labels (e.g. "Charge power",
"Period 1 start") rather than snake_case identifiers. Update tests to
match the new field_name values.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@johanzander johanzander changed the title growatt_server: fix "services" → "actions" wording in exception message growatt_server: use human-readable labels in exception messages Mar 19, 2026
@johanzander johanzander marked this pull request as ready for review March 19, 2026 20:51
Copilot AI review requested due to automatic review settings March 19, 2026 20:51
@johanzander johanzander marked this pull request as draft March 19, 2026 20:53
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 3 out of 3 changed files in this pull request and generated no new comments.

johanzander and others added 2 commits March 19, 2026 21:07
…ld wording

- Use third-person singular for all action descriptions (Reads/Writes/Updates)
- Fix write_ac_charge_times and write_ac_discharge_times device_id field
  description from "to read from" to "to write to"

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Improves readability and provides a hint to translators that these are
field names, not generic text. Dynamic placeholders are left unquoted
as ICU message format does not allow placeholders inside single quotes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@johanzander johanzander marked this pull request as ready for review March 19, 2026 21:19
Copilot AI review requested due to automatic review settings March 19, 2026 21:19
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 3 out of 3 changed files in this pull request and generated 1 comment.

johanzander and others added 2 commits March 19, 2026 21:23
…rge_times

The name field "Device" is shared with read actions so it can be
referenced rather than duplicated as a literal string. Both write
actions point directly to read_ac_charge_times for the name to avoid
chained references (Lokalise only supports one level of indirection).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All four SPH actions (read/write AC charge/discharge times) now reference
the neutral "The Growatt device to perform the action on." from
read_time_segments instead of action-specific "to read from"/"to write to"
wording. All references point directly to read_time_segments to avoid
chained references (Lokalise only supports one level of indirection).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 19, 2026 21:27
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 3 out of 3 changed files in this pull request and generated 1 comment.

@home-assistant home-assistant bot marked this pull request as draft March 19, 2026 21:41
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

…t errors

Replaces the single invalid_time_format key (which embedded a hardcoded
English field name as a placeholder) with four separate translatable keys:
invalid_time_format_start_time, invalid_time_format_end_time,
invalid_time_format_period_start, invalid_time_format_period_end.

This ensures non-English translations do not have English words embedded
in otherwise-translated error messages. Period keys use a {period}
numeric placeholder which requires no translation.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@johanzander johanzander marked this pull request as ready for review March 19, 2026 22:24
Copilot AI review requested due to automatic review settings March 19, 2026 22:24
@home-assistant home-assistant bot requested a review from joostlek March 19, 2026 22:24
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 3 out of 3 changed files in this pull request and generated 2 comments.

Comment on lines 104 to 105
return datetime.strptime(f"{parts[0]}:{parts[1]}", "%H:%M").time()
except (ValueError, IndexError) as err:
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

_parse_time_str claims to accept HH:MM:SS, and the new validation errors/messages also allow that format, but the implementation discards the seconds component by parsing only parts[0]:parts[1]. This will silently turn e.g. 11:00:30 into 11:00. Consider either parsing the full string when 3 components are provided (and validating seconds), or tightening the accepted/advertised format to HH:MM only.

Suggested change
return datetime.strptime(f"{parts[0]}:{parts[1]}", "%H:%M").time()
except (ValueError, IndexError) as err:
if len(parts) == 2:
# Format HH:MM
return datetime.strptime(time_str, "%H:%M").time()
# Format HH:MM:SS
return datetime.strptime(time_str, "%H:%M:%S").time()
except ValueError as err:

Copilot uses AI. Check for mistakes.
Comment on lines 202 to 206
end = _parse_time_str(
call.data.get(f"period_{i}_end", cached["end_time"]),
f"period_{i}_end",
"invalid_time_format_period_end",
{"period": str(i)},
)
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

New translation key path for invalid period end time (invalid_time_format_period_end with {period} placeholder) is introduced here, but the existing tests only assert the start error path. Adding a test that supplies an invalid period_1_end (and ideally one for write_ac_discharge_times too) would prevent future regressions in the raised translation key/placeholders.

Copilot uses AI. Check for mistakes.
Co-authored-by: Norbert Rittel <norbert@rittel.de>
Copilot AI review requested due to automatic review settings March 20, 2026 07:30
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 3 out of 3 changed files in this pull request and generated 2 comments.

Comment on lines +616 to +620
"invalid_time_format_period_end": {
"message": "'Period {period} end' must be in HH:MM or HH:MM:SS format."
},
"invalid_time_format_period_start": {
"message": "'Period {period} start' must be in HH:MM or HH:MM:SS format."
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

strings.json is currently invalid JSON in this hunk: the invalid_time_format_period_start entry is missing its closing } (and comma), and another key starts immediately after it. This will break translation loading/build; please fix the structure so each exception key is a valid, properly closed JSON object.

Suggested change
"invalid_time_format_period_end": {
"message": "'Period {period} end' must be in HH:MM or HH:MM:SS format."
},
"invalid_time_format_period_start": {
"message": "'Period {period} start' must be in HH:MM or HH:MM:SS format."
},

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Github somehow messed this up when I just added the two single quotes on each line.

Comment on lines +616 to +620
"invalid_time_format_period_end": {
"message": "'Period {period} end' must be in HH:MM or HH:MM:SS format."
},
"invalid_time_format_period_start": {
"message": "'Period {period} start' must be in HH:MM or HH:MM:SS format."
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

There are duplicate exception keys (invalid_time_format_period_start / invalid_time_format_period_end) defined twice here. Also, one of the duplicates wraps the {period} placeholder inside single quotes ('Period {period} …'), which ICU message formatting treats as escaped text (the placeholder may not be substituted). Keep only one definition per key and ensure placeholders are not inside quoted segments.

Suggested change
"invalid_time_format_period_end": {
"message": "'Period {period} end' must be in HH:MM or HH:MM:SS format."
},
"invalid_time_format_period_start": {
"message": "'Period {period} start' must be in HH:MM or HH:MM:SS format."
},

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Using two single quotes in those four places should fix that issue.

Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

The PR template isn't there and CI is failing, can you take a look?

@home-assistant home-assistant bot marked this pull request as draft March 20, 2026 15:18
johanzander and others added 2 commits March 20, 2026 15:29
The GitHub UI suggestion added quoted period field names without removing
the originals or closing their braces, resulting in duplicate keys and
invalid JSON. Keep only the quoted versions: 'Period {period} end' and
'Period {period} start', consistent with other field name messages.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dd end time test

- Fix _parse_time_str to actually parse seconds when HH:MM:SS format is
  provided, instead of silently discarding them
- Add test for invalid period end time (invalid_time_format_period_end)
  to complement the existing period start time test

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 20, 2026 15:37
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 3 out of 3 changed files in this pull request and generated no new comments.

@johanzander johanzander marked this pull request as ready for review March 20, 2026 15:46
@home-assistant home-assistant bot requested a review from joostlek March 20, 2026 15:47
@johanzander
Copy link
Contributor Author

This should hopefully fix the PR template issue: #166069

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.

4 participants