Skip to content

Replace f-string usage in _LOGGER calls with printf style - #2272

Merged
wills106 merged 19 commits into
wills106:mainfrom
TCWORLD:remove-log-f-strings
Aug 20, 2026
Merged

Replace f-string usage in _LOGGER calls with printf style#2272
wills106 merged 19 commits into
wills106:mainfrom
TCWORLD:remove-log-f-strings

Conversation

@TCWORLD

@TCWORLD TCWORLD commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

The problem with using f-strings in _LOGGER calls is that even if logging for that message level is disabled, formatting of the strings will still be performed.

For infrequent logs that's a non-issue, but for e.g. auto-repeat loops or sensor calculations where this is happening every couple of seconds with multiple logs, it is potentially a lot of wasted processing time.

Using the printf style means the formatting will not be performed if debug logging is disabled.

This PR enables Ruff rule G004 which demands no f-strings in _LOGGER calls, and replaces all such usages with standard printf style strings.

Fixes #2023


Review Note:

While I've tried to take a lot of care in doing this, and checked as I went along, this was a manual process so its entirely possible that I've broken something in the process. We should check this PR thoroughly before merging it to try and catch any mistakes.

To aid this, I've tried to split up the commit history into a plugin by plugin basis to try and break it down into bitesize chunks.

Might be worth getting a LLM bot to review this as well.

@TCWORLD
TCWORLD marked this pull request as ready for review August 9, 2026 12:32
@wills106

Copy link
Copy Markdown
Owner

Might be worth getting a LLM bot to review this as well.

I didn't include your last commit (57b1f61)

But I managed to get the rest of the diff change into an LLM. Not looked into it too much yet, but this is the output:

Review summary

The diff is mostly a safe conversion from f-strings to lazy logger formatting, but it introduces two definite logging errors and a few behavioural changes that should be reviewed before merging.

Definite errors

1. Incorrect logger arguments in async_unload_entry

In custom_components/solax_modbus/__init__.py, the two replacements below are incorrect:

_LOGGER.warning(": error removing from hass.data: ", name, ex)
_LOGGER.debug(": Energy Dashboard coordinator cleanup failed: ", name, ex)

The format strings contain no placeholders, but name and ex are still passed as logging arguments. Python logging will attempt to apply the arguments using % formatting and can emit a secondary logging error such as:

TypeError: not all arguments converted during string formatting

The original messages included both the hub name and exception, so these are also information-loss regressions.

They should be equivalent to:

_LOGGER.warning("%s: error removing from hass.data: %s", name, ex)
_LOGGER.debug("%s: Energy Dashboard coordinator cleanup failed: %s", name, ex)

These are the most important issues in the diff.

2. Accidental set construction in button.py

This replacement changes the logged value:

_LOGGER.info("hub.wakeuButton: %s", {hub.wakeupButton})

The braces create a set, rather than passing the value directly. Consequently, a value such as "wakeup" will be logged as:

{'wakeup'}

It should be:

_LOGGER.info("hub.wakeuButton: %s", hub.wakeupButton)

This will normally only affect log output, assuming hub.wakeupButton is hashable. If it can ever be an unhashable value, the set construction could raise an exception before logging.

The existing spelling wakeuButton appears to be pre-existing, but could also be corrected while touching this line.

Behavioural changes beyond logging

3. New conversion helpers can raise exceptions

const.py adds:

def value_int_default(val: Any, default: int) -> int:
    return int(val) if val is not None else default

This differs from common previous patterns such as:

datadict.get("some_key", 0) or 0

For example:

  • None becomes the default, as intended.
  • 0 is converted to 0.
  • "123" becomes 123.
  • "abc" raises ValueError.
  • An unexpected object can raise TypeError.
  • A float is truncated, for example 12.9 becomes 12.

This is not inherently wrong, and it appears intended to distinguish None from valid zero values, but the new helper should only be used where the input is guaranteed to be numeric or already validated. The affected control functions are safety-relevant because they calculate inverter charge, discharge, import, and export commands.

At minimum, tests should cover None, 0, numeric strings, floats, and unexpected values.

4. measured_power handling has changed from “missing” to zero

In plugin_solax.py, some code changes from preserving a missing value:

measured_power = datadict.get("measured_power", None)

to:

measured_power = value_int_default(datadict.get("measured_power"), 0)

This means a missing or None measurement is now treated as 0 W, rather than being retained as unavailable. That may be intentional, but it can alter control decisions in the Mode 8 algorithms. A missing grid measurement could therefore be interpreted as no measured import/export rather than an unavailable input.

The corresponding plugin_solax_a1j1.py and plugin_solax_lv.py sections still use the old None/"n/a" behaviour, so the implementations are now inconsistent between plugins.

5. Potentially changed fallback semantics for falsy values

Several expressions were changed from patterns such as:

int(datadict.get("value", 100) or 100)

to:

value_int_default(datadict.get("value"), 100)

This deliberately preserves zero instead of replacing it with the fallback. That is likely the purpose of the change, but it is a functional change, not merely a logging refactor.

Examples include:

  • negative_injection_bias_w
  • export_feedback_deadband_w
  • pv_unlimited_delta_w
  • export_first_export_margin_w
  • battery_power_charge
  • remotecontrol_current_pv_power_limit
  • remotecontrol_current_pushmode_power

For each setting, confirm whether zero is a valid configured value. In particular, a zero deadband or zero margin may be valid, whereas a zero step size could potentially prevent the controller from making progress.

Formatting conversion checks

The majority of the logger conversions appear correctly implemented:

  • %s, %d, %x, %f, and escaped %% are generally used appropriately.
  • exc_info=True is still passed correctly in the affected exception handlers.
  • The conversions preserve lazy formatting, which is preferable for logging performance.
  • Replacing en dashes with hyphens in log messages is harmless.

I did not identify other obvious placeholder/count mismatches in the supplied diff, but this type of issue is worth checking automatically because the diff is very large.

Recommended automated checks

Run the following before merging:

ruff check custom_components/solax_modbus
mypy custom_components/solax_modbus
pytest

If available, enable a test or lint rule that detects logging calls with mismatched format arguments. A simple targeted test should also exercise the two exception paths in async_unload_entry, because ordinary tests may not trigger them.

Verdict

I would not merge the diff unchanged. Fix these two definite issues first:

  • The two logger calls in async_unload_entry with missing %s placeholders.
  • The set literal around hub.wakeupButton.

Then review the new numeric conversion behaviour, especially the treatment of missing measured_power as zero and the effect of preserving configured zero values in the Mode 8 control logic.

TCWORLD added 17 commits August 20, 2026 09:28
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
To account for new Ruff G004 rule.
Couple of missing %s were missed when switching, and a set of {} were not removed.
@TCWORLD
TCWORLD force-pushed the remove-log-f-strings branch from ab81188 to 5109946 Compare August 20, 2026 08:32
@TCWORLD

TCWORLD commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Cool. I'm actually quite surprised how few mistakes there were in that change!
I've pushed a commit with the corrected log messages.

As for the default functions (points 4 and 5), they weren't supposed to be in this PR, I must have based it on a working branch rather than main.
I have rebased the PR to remove those unrelated changes for now so this PR just concerns log message changes. I'll introduce them in a separate PR later.

Lost the return None in energy dashboard.
Fix CI lint check in plugin_solax.py
@wills106
wills106 merged commit 4324eb9 into wills106:main Aug 20, 2026
13 checks passed
anton4 pushed a commit to anton4/homeassistant-solax-modbus that referenced this pull request Aug 21, 2026
Upstream enabled ruff G004 (no f-strings in logging) in pyproject.toml and
converted its own logging in PR wills106#2272, merged the day before this sync. Only one
of our log calls trips it - the Full-write fallback error, which passed the
f-string directly. The rest build their message into a variable first, because
_remote_power_warn dedupes on the message text, so lazy formatting does not apply
there.

Also drops a stray blank line in NUMBER_TYPES that ruff format wanted; it came in
with d3dd73d and predates the sync, so the tree is now format-clean too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Suggestion] Use printf format not f-strings for debug logs

2 participants