chore(lint): enable ruff F841 (unused local variable) - #4781
Merged
Conversation
Mechanical `except X as e:` -> `except X:` where e was never read, identified by ruff's F841. Part of the #2198 linter-complaint cleanup - F841 itself has 123 more hits needing individual judgment, not yet enabled in .pre-commit-config.yaml.
Almost all were `result = asyncio.run(...)` where only the side effect mattered, not the return value - dropped the assignment, kept the call. Two mock context-manager captures (mock_sleep, mock_log) were never asserted on, so dropped the `as` binding. One (msg) was fully dead. Part of the #2198 F841 cleanup.
battery_capacity/battery_voltage locals in update_status() were dead initialisers never read anywhere in the loop or after. The 13 REST `r = self.rest_postCommand(...)` sites only cared about the POST's side effect - verification happens via a separate rest_runAll() re-fetch - so dropped the unused assignment. Part of the #2198 F841 cleanup.
soc_min/soc_max/soc_percent_min/soc_percent_max in the per-slot plan row builder duplicated the soc_min_window/soc_max_window block right below it - same loop range, same computation, and the non-window soc_percent_* was never read anywhere. iboost_slot_end was a dead write. soc_kw in calculate_yesterday()'s save-state block was dead too: unlike its siblings, self.soc_kw is never mutated in the "fake to yesterday" block, so there was nothing to restore. Part of the #2198 F841 cleanup.
soc_percent_min (per-slot plan builder), window_length_orig (export window swap loop), and best_soc (optimise_levels_pass init) were all dead - none read anywhere else in their function, and best_soc wasn't even part of the tuple its siblings get reassigned through. Verified against the random-scenario baseline plus optimise_levels/window/swap tests. Part of the #2198 F841 cleanup.
dayname/daysymbol and day_of_week were regex capture groups extracted but never read by the callers they feed. Part of the #2198 F841 cleanup.
Mostly `result = ha_interface.some_call(...)` where only the mocked side effect was checked, not the return value - dropped the assignment, kept the call. original_wait/original_count were dead manual-backup locals never restored or read (the real monkeypatch in the same test uses patch.object() and cleans up on its own). Part of the #2198 F841 cleanup.
Mostly dead locals (init'd, never read - e.g. db_engine.py's last_keep, prediction.py's iboost_freeze whose actual effect is a separate discharge_rate_now assignment) or side-effecting calls whose return value was never used (ohme.py's async_set_vehicle, web.py's get_component lookup). compare.py's import_url was a genuine duplicate resolve_arg() call, reused instead of dropped. web_mcp.py's unverified JWT decode was missing the log line its own comment promised - added it rather than deleting the decode. web_mcp.py's client_secret is unused by design: this OAuth endpoint is PKCE-only per the MCP spec, not a confidential-client flow. Part of the #2198 F841 cleanup.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces potentially sensitive JWT-claim logging on auth failures and also contains misleading inverter error logs with swapped format arguments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Enables Ruff’s F841 rule (unused local variables) and applies mechanical cleanups across the codebase to remove now-flagged dead locals and unused exception bindings, aiming to improve lint hygiene without changing runtime behaviour.
Changes:
- Enable Ruff F841 and remove/adjust unused locals across core modules (planning, prediction, inverter control, integrations, web).
- Simplify exception handlers where the exception binding wasn’t used.
- Minor targeted cleanup/improvement while fixing F841 hits (e.g., reuse precomputed
import_url, add JWT “unverified claims” debug logging).
File summaries
| File | Description |
|---|---|
| apps/predbat/web.py | Removes unused local and unused exception binding in task-stack inspection. |
| apps/predbat/web_mcp.py | Adds logging of unverified JWT claims on verification failure; removes unused client_secret local. |
| apps/predbat/utils.py | Removes unused exception binding and dead locals in minute-data helpers. |
| apps/predbat/userinterface.py | Removes unused local from config enablement logic. |
| apps/predbat/tests/test_hainterface_service.py | Drops unused return capture and removes unused original_wait local. |
| apps/predbat/tests/test_hainterface_lifecycle.py | Removes unused locals/comments in lifecycle timeout test. |
| apps/predbat/tests/test_hainterface_api.py | Drops unused return captures in API call tests. |
| apps/predbat/tests/test_hahistory.py | Drops unused return captures in history cache tests. |
| apps/predbat/tests/test_fox_api.py | Drops unused return captures and unused locals/mocks from Fox API tests. |
| apps/predbat/prediction.py | Removes unused iboost_freeze local. |
| apps/predbat/predheat.py | Removes unused energy accumulator local. |
| apps/predbat/plan.py | Removes unused locals in scenario summary / optimisation passes. |
| apps/predbat/output.py | Removes unused SOC/iboost locals and redundant SOC min/max computation block. |
| apps/predbat/ohme.py | Drops unused return capture from a PUT request during vehicle selection. |
| apps/predbat/octopus.py | Removes unused regex group locals and unused exception binding. |
| apps/predbat/load_predictor.py | Removes unused normalized target local in training flow. |
| apps/predbat/load_ml_component.py | Removes unused local in publish path. |
| apps/predbat/inverter.py | Removes unused locals and unused exception binding; drops unused REST POST response assignments. |
| apps/predbat/gecloud.py | Removes unused exception binding on JSON decode / connection failures. |
| apps/predbat/futurerate.py | Removes unused “prev_*” locals during rate extraction. |
| apps/predbat/fox.py | Removes unused entity-name locals in publish. |
| apps/predbat/fetch.py | Removes unused locals in rate replication and scanning logic. |
| apps/predbat/db_engine.py | Removes unused local from last-record unpacking. |
| apps/predbat/compare.py | Reuses computed import_url instead of recomputing inline. |
| apps/predbat/carbon.py | Removes unused exception binding during parsing. |
| apps/predbat/alertfeed.py | Removes unused locals in alert application flow. |
Review details
Suppressed comments (1)
apps/predbat/inverter.py:1882
- Same as above for the charge-rate path: the log message's format arguments are reversed, so the inverter ID and invalid value are swapped in the output.
try:
current_rate = int(current_rate)
except (ValueError, TypeError):
self.base.log("Error: Inverter {} charge rate {} is not a number, setting to {}W".format(current_rate, self.id, self.battery_rate_max_raw))
current_rate = self.battery_rate_max_raw
- Files reviewed: 26/26 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
226
to
231
| # Try to decode without verification to see what's in the token (for debugging) | ||
| try: | ||
| unverified = pyjwt.decode(token, options={"verify_signature": False}) | ||
| self.log(f"MCP: Token claims (unverified): {unverified}") | ||
| except Exception as e2: | ||
| self.log(f"MCP: Could not even decode without verification: {e2}") |
Comment on lines
+1861
to
1863
| except (ValueError, TypeError): | ||
| self.base.log("Error: Inverter {} charge discharge {} is not a number, setting to {}W".format(current_rate, self.id, self.battery_rate_max_raw)) | ||
| current_rate = self.battery_rate_max_raw |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enables ruff's F841 rule (local variable assigned but never used) and fixes all 132 sites it caught. Stacked on #4780, last in the rule-by-rule cleanup from #2198.
Closes #2198
Summary
--fix(unused exception-bindingas e)compare.py(import_urlcomputed then immediately recomputed inline) - reused instead of droppedweb_mcp.py(unverifiedJWT decode) - added the log line its own comment promised, instead of deleting itweb_mcp.py'sclient_secretis unused by design: that OAuth endpoint is PKCE-only per the MCP spec, not a confidential-client flowTouches
inverter.py,plan.py,output.py,octopus.py,fetch.py,fox.py,futurerate.py,ohme.py,predheat.py,prediction.py,db_engine.py,compare.py,userinterface.py,utils.py,web.py,web_mcp.py,load_ml_component.py,load_predictor.py, and several test files.While auditing this rule I also found a real bug in
sigenergy.py(a configured export rate that was silently never applied) - pulled that out into its own PR (#4782) rather than bundling a behaviour change into this lint stack.Test plan
./run_pre_commitpasses