refactor: add typing to everything#601
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
|
Looks like there's some mypy issues to fix. |
secondof9
reviewed
Jun 2, 2026
secondof9
left a comment
secondof9
left a comment
There was a problem hiding this comment.
Hermes Agent Review
Found 1 warning and 5 suggestions. See inline comments below.
Code Review SummaryVerdict: Changes Requested 🔴 (1 warning, 5 suggestions) PR: #601 — Add typing to everything 🔴 CriticalNone.
|
Contributor
Author
|
@firstof9 - Fixed. |
firstof9
added
refactor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openevsehttp/websocket.py (1)
87-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFallback uses deprecated
asyncio.get_event_loop()and has flawed logic.If
ensure_futurefails because there's no running event loop, callingget_event_loop()andcall_soon_threadsafewon't help—that loop isn't running either. The callback won't execute, andget_event_loop()is deprecated in Python 3.10+ when called outside an async context.Consider removing this fallback entirely and letting the outer
except RuntimeErrorhandle it, or document that the setter must only be called from an async context or with_listener_loopset.Proposed simplification
try: if self._listener_loop: self._listener_loop.call_soon_threadsafe(self._schedule_task, coro) else: - try: - task = asyncio.ensure_future(coro) - self._tasks.add(task) - task.add_done_callback(self._tasks.discard) - except RuntimeError: - # Fallback to get_event_loop if ensure_future fails and no _listener_loop - loop = asyncio.get_event_loop() - loop.call_soon_threadsafe(self._schedule_task, coro) + task = asyncio.ensure_future(coro) + self._tasks.add(task) + task.add_done_callback(self._tasks.discard) except RuntimeError: _LOGGER.error("Failed to schedule callback from sync context: %s", coro) if hasattr(coro, "close"): coro.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openevsehttp/websocket.py` around lines 87 - 90, The fallback in the except RuntimeError block uses asyncio.get_event_loop() (deprecated) and call_soon_threadsafe on a non-running loop, which is incorrect; remove this fallback and either re-raise the RuntimeError or document that the setter must be invoked from an async context or with _listener_loop populated. Specifically, update the except RuntimeError handling around asyncio.ensure_future(...) in websocket.py: remove the call to asyncio.get_event_loop() and loop.call_soon_threadsafe(self._schedule_task, coro), and ensure the logic relies on the existing outer exception handling or clearly enforces/validates that _listener_loop is set before scheduling (referencing ensure_future, _schedule_task, and _listener_loop).openevsehttp/client.py (1)
298-300:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeepalive exits before the socket ever reaches
connected.
_start_listening()schedulesrepeat()while_ws_listeningis stillFalse, andrepeat()uses that flag in itswhilecondition. On a cold start the task returns immediately, so the websocket never sends periodic keepalives after it does connect.Suggested fix
async def repeat( self, interval: float, func: Callable[..., Any], *args: Any, **kwargs: Any, ) -> None: @@ - while self.ws_state != STATE_STOPPED and self._ws_listening: + while self.ws_state != STATE_STOPPED: await asyncio.sleep(interval) - if self.ws_state == STATE_STOPPED or not self._ws_listening: + if self.ws_state == STATE_STOPPED: break + if not self._ws_listening: + continue result = func(*args, **kwargs) if inspect.isawaitable(result): await resultAlso applies to: 450-456
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openevsehttp/client.py` around lines 298 - 300, The keepalive task is being created while _ws_listening is still False so repeat() immediately exits; ensure the keepalive loop is started only after the websocket is considered listening/connected. Fix by moving creation of self._ws_keepalive_task (currently creating repeat(300, self.websocket.keepalive)) to after _ws_listening is set True or by changing repeat()'s condition to wait for websocket.connected (or await a websocket.wait_until_connected() helper) before entering its while loop; update both occurrences that schedule the task (the _start_listening() spot and the second occurrence around lines 450-456) and reference _start_listening, repeat, _ws_keepalive_task, _ws_listening, and websocket.keepalive when applying the change.
🧹 Nitpick comments (2)
tests/test_client.py (1)
1728-1737: ⚡ Quick winParametrize the primitive rejection cases.
This only covers numeric primitives. The parser also rejects
falseandnull, and those are easy to regress because plain text and JSON strings are still allowed.Suggested test shape
-async def test_process_request_invalid_json_primitive(mock_aioclient): - """Test process_request with an unexpected JSON primitive (e.g., bool or int).""" - charger = OpenEVSE(SERVER_URL) - mock_aioclient.get( - TEST_URL_STATUS, - status=200, - body="123", - ) - with pytest.raises(ParseJSONError): - await charger.process_request(TEST_URL_STATUS, method="get") +@pytest.mark.parametrize("body", ["123", "false", "null"]) +async def test_process_request_invalid_json_primitive(mock_aioclient, body): + """Test process_request rejects unexpected JSON primitives.""" + charger = OpenEVSE(SERVER_URL) + mock_aioclient.get( + TEST_URL_STATUS, + status=200, + body=body, + ) + with pytest.raises(ParseJSONError): + await charger.process_request(TEST_URL_STATUS, method="get")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_client.py` around lines 1728 - 1737, The test test_process_request_invalid_json_primitive only checks a numeric primitive; update it to parametrize multiple JSON primitive responses (e.g., "123", "false", "null") so process_request on OpenEVSE with method="get" raises ParseJSONError for each primitive; locate the test function name test_process_request_invalid_json_primitive and the call to charger.process_request and replace the single-case mock_aioclient.get + assert with a pytest.mark.parametrize over the primitive bodies, asserting ParseJSONError for each.openevsehttp/properties.py (1)
128-128: ⚡ Quick winUse real type expressions in
typing.cast()(no quoted type strings)
openevsehttp/properties.pyusescast("...")with quoted type targets (e.g.,cast("int | None", ...),cast("float | None", ...), etc.) at lines 128, 306-321, 400, 421, 426, 439, 497, 502, 507, 512. Switch tocast(int | None, ...),cast(float | None, ...),cast(str | None, ...), etc.Representative fix
- return cast("int | None", self._status.get("max_current", None)) + return cast(int | None, self._status.get("max_current", None)) - return cast("float | None", self._status.get("total_day", None)) + return cast(float | None, self._status.get("total_day", None)) - return cast("str | None", self._config.get("wifi_serial", None)) + return cast(str | None, self._config.get("wifi_serial", None))wifi_firmware’s early-return on missing
versionmatches the tests, so that part is fine as-is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openevsehttp/properties.py` at line 128, Replace the string-literal type arguments passed to typing.cast with real type expressions (use PEP 604 unions like int | None, float | None, str | None) wherever cast is called; e.g., change cast("int | None", self._status.get("max_current", None)) to cast(int | None, self._status.get("max_current", None)) and make the equivalent replacements for the other occurrences (the cast calls around the status/accessor properties — the similar casts at the block handling voltage/temperature/firmware/version and the other listed cast sites). Ensure you keep the same return values and imports (no code logic changes), only replace the quoted type strings with actual type expressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openevsehttp/client.py`:
- Around line 298-300: The keepalive task is being created while _ws_listening
is still False so repeat() immediately exits; ensure the keepalive loop is
started only after the websocket is considered listening/connected. Fix by
moving creation of self._ws_keepalive_task (currently creating repeat(300,
self.websocket.keepalive)) to after _ws_listening is set True or by changing
repeat()'s condition to wait for websocket.connected (or await a
websocket.wait_until_connected() helper) before entering its while loop; update
both occurrences that schedule the task (the _start_listening() spot and the
second occurrence around lines 450-456) and reference _start_listening, repeat,
_ws_keepalive_task, _ws_listening, and websocket.keepalive when applying the
change.
In `@openevsehttp/websocket.py`:
- Around line 87-90: The fallback in the except RuntimeError block uses
asyncio.get_event_loop() (deprecated) and call_soon_threadsafe on a non-running
loop, which is incorrect; remove this fallback and either re-raise the
RuntimeError or document that the setter must be invoked from an async context
or with _listener_loop populated. Specifically, update the except RuntimeError
handling around asyncio.ensure_future(...) in websocket.py: remove the call to
asyncio.get_event_loop() and loop.call_soon_threadsafe(self._schedule_task,
coro), and ensure the logic relies on the existing outer exception handling or
clearly enforces/validates that _listener_loop is set before scheduling
(referencing ensure_future, _schedule_task, and _listener_loop).
---
Nitpick comments:
In `@openevsehttp/properties.py`:
- Line 128: Replace the string-literal type arguments passed to typing.cast with
real type expressions (use PEP 604 unions like int | None, float | None, str |
None) wherever cast is called; e.g., change cast("int | None",
self._status.get("max_current", None)) to cast(int | None,
self._status.get("max_current", None)) and make the equivalent replacements for
the other occurrences (the cast calls around the status/accessor properties —
the similar casts at the block handling voltage/temperature/firmware/version and
the other listed cast sites). Ensure you keep the same return values and imports
(no code logic changes), only replace the quoted type strings with actual type
expressions.
In `@tests/test_client.py`:
- Around line 1728-1737: The test test_process_request_invalid_json_primitive
only checks a numeric primitive; update it to parametrize multiple JSON
primitive responses (e.g., "123", "false", "null") so process_request on
OpenEVSE with method="get" raises ParseJSONError for each primitive; locate the
test function name test_process_request_invalid_json_primitive and the call to
charger.process_request and replace the single-case mock_aioclient.get + assert
with a pytest.mark.parametrize over the primitive bodies, asserting
ParseJSONError for each.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f43340a-68ee-4429-b953-f1fe2b800b0c
📒 Files selected for processing (9)
openevsehttp/client.pyopenevsehttp/commands.pyopenevsehttp/properties.pyopenevsehttp/py.typedopenevsehttp/websocket.pypyproject.tomltests/test_client.pytests/test_commands.pytests/test_properties.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openevsehttp/client.py (1)
323-331:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
_ws_listeningon everySTATE_STOPPEDtransition.A clean stop currently leaves
_ws_listeningunchanged. If the socket reachesSTATE_STOPPEDwithout an error,update()will still treat websocket updates as live and skip/statuspolling, leaving status stale until a full reconnect/disconnect cycle resets the flag.Suggested fix
- elif data == STATE_STOPPED and error: - _LOGGER.debug( - "Websocket to %s failed, aborting [Error: %s]", - uri, - error, - ) - self._ws_listening = False + elif data == STATE_STOPPED: + if error: + _LOGGER.debug( + "Websocket to %s failed, aborting [Error: %s]", + uri, + error, + ) + self._ws_listening = False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openevsehttp/client.py` around lines 323 - 331, The code only clears self._ws_listening when STATE_STOPPED occurs with an error; change the STATE_STOPPED handling so that self._ws_listening is set to False for every STATE_STOPPED transition (regardless of the error value) in the websocket handler in client.py (the block that currently checks "elif data == STATE_STOPPED and error"); either remove the "and error" from that condition or add a branch that sets self._ws_listening = False when data == STATE_STOPPED without error so update() will resume polling /status.
🧹 Nitpick comments (1)
tests/test_websocket.py (1)
291-328: ⚡ Quick winKeep one success-path test for cross-thread state scheduling.
These additions cover the
callback is Nonebranch and the local scheduling failure path, but they no longer exercise a successful state change from outside the listener loop. Since this refactor hardens thread-safe callback scheduling, keeping one positive cross-thread case would catch regressions in the path that matters most.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_websocket.py` around lines 291 - 328, Add one positive cross-thread scheduling test to cover the successful path for state changes originating outside the listener loop: create a test (e.g., test_websocket_schedule_success_sync) that assigns an awaitable AsyncMock to ws_client.callback, simulates setting ws_client.state from a non-event-loop context (so the code path in the state setter that calls asyncio.ensure_future is taken), and asserts the callback was scheduled/awaited and no error was logged or stored in ws_client._error_reason; reference the ws_client.state setter, the callback attribute, and ensure_future scheduling logic to locate the code to exercise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openevsehttp/client.py`:
- Around line 323-331: The code only clears self._ws_listening when
STATE_STOPPED occurs with an error; change the STATE_STOPPED handling so that
self._ws_listening is set to False for every STATE_STOPPED transition
(regardless of the error value) in the websocket handler in client.py (the block
that currently checks "elif data == STATE_STOPPED and error"); either remove the
"and error" from that condition or add a branch that sets self._ws_listening =
False when data == STATE_STOPPED without error so update() will resume polling
/status.
---
Nitpick comments:
In `@tests/test_websocket.py`:
- Around line 291-328: Add one positive cross-thread scheduling test to cover
the successful path for state changes originating outside the listener loop:
create a test (e.g., test_websocket_schedule_success_sync) that assigns an
awaitable AsyncMock to ws_client.callback, simulates setting ws_client.state
from a non-event-loop context (so the code path in the state setter that calls
asyncio.ensure_future is taken), and asserts the callback was scheduled/awaited
and no error was logged or stored in ws_client._error_reason; reference the
ws_client.state setter, the callback attribute, and ensure_future scheduling
logic to locate the code to exercise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b183062d-6e83-44bf-9344-fc1bbd1a0566
📒 Files selected for processing (5)
openevsehttp/client.pyopenevsehttp/properties.pyopenevsehttp/websocket.pytests/test_client.pytests/test_websocket.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openevsehttp/websocket.py
Hermes Agent Code Review — PR #601Verdict: Changes Requested (5 suggestions, 2 test improvements) 🔵 Suggestions
|
Owner
|
Thanks @c00w |
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
3 participants
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.
This adds strict typing, and fixes some type gaps in the library.
Summary by CodeRabbit
Bug Fixes
Tests
Chores