Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds async OpenEVSE.set_shaper_live_pwr(power: int) to POST Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant OpenEVSE
participant ChargerAPI
Caller->>OpenEVSE: call set_shaper_live_pwr(power)
OpenEVSE->>OpenEVSE: check firmware version
alt firmware >= 4.0.0
OpenEVSE->>ChargerAPI: POST /status { "shaper_live_pwr": power }
ChargerAPI-->>OpenEVSE: HTTP response
OpenEVSE-->>Caller: return / log response
else older firmware
OpenEVSE-->>Caller: raise UnsupportedFeature / log not supported
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.4)tests/test_main.pyThanks 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
openevsehttp/__main__.py (2)
1316-1316: Method name and parameter name are identical — rename the parameter.
shaper_live_pwr(self, shaper_live_pwr: int)uses the same identifier for both the method and its parameter. While Python resolves it correctly at runtime (the parameter shadows the method name inside the body), it harms readability and can cause confusion when reading or calling the method.♻️ Suggested rename
- async def shaper_live_pwr(self, shaper_live_pwr: int) -> None: - """Send pushed sensor data to shaper.""" - url = f"{self.url}status" - data = {"shaper_live_pwr": shaper_live_pwr} + async def shaper_live_pwr(self, value: int) -> None: + """Send pushed sensor data to shaper.""" + url = f"{self.url}status" + data = {"shaper_live_pwr": value}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/__main__.py` at line 1316, The method shaper_live_pwr currently uses the same identifier for the method and its parameter, which is confusing; rename the parameter (for example to value, power, or new_shaper_live_pwr) in the async def shaper_live_pwr(...) signature, update the type hint accordingly, and adjust all references inside the method body to the new parameter name; also update any callers, docstrings, and tests that pass that parameter to use the new name to ensure consistency.
1315-1323: Add version guard or document intentional omission.
shaper_live_pwrlacks the_version_check()guard present in sibling POST methods (grid_voltage,self_production,soc). Test fixtures show shaper is unavailable in firmware 2.9.1 but available in 4.1.2. Either add a guard (likely_version_check("4.0.0")or_version_check("4.1.0")based on patterns) or include a comment explaining why the feature is universally supported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/__main__.py` around lines 1315 - 1323, The shaper_live_pwr method is missing the firmware version guard used by sibling POST methods (grid_voltage, self_production, soc); add a call to _version_check (e.g., _version_check("4.1.0") or whichever minimum version matches other shaper-related endpoints) at the start of shaper_live_pwr to early-return or raise when unsupported, or add a clear comment above shaper_live_pwr explaining that this endpoint is intentionally available for all firmware versions; reference the shaper_live_pwr method and the existing _version_check utility when making the change.tests/test_main.py (2)
1274-1286: Test only covers the happy path — consider adding an edge-case scenario.Comparable tests (
test_soc,test_self_production) validate at least one additional scenario (empty data,UnsupportedFeature, invalid input). This test only verifies a single successful POST. At minimum, testing a zero or negative value would ensure the method (or a future validation guard) behaves as expected at the boundaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_main.py` around lines 1274 - 1286, Add an edge-case test for shaper_live_pwr in addition to the happy path: update or create a new test (e.g., alongside test_set_shaper_live_power) that calls test_charger.shaper_live_pwr with a boundary value such as 0 (and optionally a negative value) and asserts the expected behavior — for example that it posts the correct payload, handles an empty response/body, or raises/handles UnsupportedFeature or a validation error depending on implementation; use the same mock_aioclient.post to simulate the service returning either an empty body, an error status, or a payload reflecting the boundary value and assert the corresponding log messages or exceptions to mirror the existing checks in test_set_shaper_live_power.
1281-1281:repeat=Trueis unnecessary — the method is called exactly once in this test.♻️ Proposed fix
mock_aioclient.post( TEST_URL_STATUS, status=200, body='{"shaper_live_pwr": 210}', - repeat=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_main.py` at line 1281, Remove the unnecessary repeat=True argument from the call in tests/test_main.py (the call that currently includes "repeat=True"); locate the invocation that passes repeat=True and delete that keyword argument so the method is called with its default single-invocation behavior (no other code changes required unless tests assert repeated behavior, in which case adjust those assertions accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openevsehttp/__main__.py`:
- Line 1316: The method shaper_live_pwr currently uses the same identifier for
the method and its parameter, which is confusing; rename the parameter (for
example to value, power, or new_shaper_live_pwr) in the async def
shaper_live_pwr(...) signature, update the type hint accordingly, and adjust all
references inside the method body to the new parameter name; also update any
callers, docstrings, and tests that pass that parameter to use the new name to
ensure consistency.
- Around line 1315-1323: The shaper_live_pwr method is missing the firmware
version guard used by sibling POST methods (grid_voltage, self_production, soc);
add a call to _version_check (e.g., _version_check("4.1.0") or whichever minimum
version matches other shaper-related endpoints) at the start of shaper_live_pwr
to early-return or raise when unsupported, or add a clear comment above
shaper_live_pwr explaining that this endpoint is intentionally available for all
firmware versions; reference the shaper_live_pwr method and the existing
_version_check utility when making the change.
In `@tests/test_main.py`:
- Around line 1274-1286: Add an edge-case test for shaper_live_pwr in addition
to the happy path: update or create a new test (e.g., alongside
test_set_shaper_live_power) that calls test_charger.shaper_live_pwr with a
boundary value such as 0 (and optionally a negative value) and asserts the
expected behavior — for example that it posts the correct payload, handles an
empty response/body, or raises/handles UnsupportedFeature or a validation error
depending on implementation; use the same mock_aioclient.post to simulate the
service returning either an empty body, an error status, or a payload reflecting
the boundary value and assert the corresponding log messages or exceptions to
mirror the existing checks in test_set_shaper_live_power.
- Line 1281: Remove the unnecessary repeat=True argument from the call in
tests/test_main.py (the call that currently includes "repeat=True"); locate the
invocation that passes repeat=True and delete that keyword argument so the
method is called with its default single-invocation behavior (no other code
changes required unless tests assert repeated behavior, in which case adjust
those assertions accordingly).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_main.py (1)
1293-1295: Missing response log assertion for the zero-power case.The first scenario (power=210, lines 1283-1286) asserts both the posting and the response log, but the zero-power path only checks the posting log. For consistency and completeness, assert the response as well.
♻️ Proposed addition
with caplog.at_level(logging.DEBUG): await test_charger.set_shaper_live_pwr(0) assert "Posting shaper data: {'shaper_live_pwr': 0}" in caplog.text + assert "Shaper response: {'shaper_live_pwr': 0}" in caplog.text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_main.py` around lines 1293 - 1295, The zero-power test block is missing the assertion that the response log contains the expected response message; after calling test_charger.set_shaper_live_pwr(0) and asserting the posting log, add an assertion that caplog.text includes the response log string used elsewhere (the same response-check pattern as the 210W case) to verify the response was logged; locate the zero-power block around the call to test_charger.set_shaper_live_pwr and mirror the response assertion used with the 210 case to keep tests consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_main.py`:
- Around line 1297-1302: The pytest.raises block currently wraps
test_charger_v2.update(), set_shaper_live_pwr(210), the caplog assertion, and
ws_disconnect(), but set_shaper_live_pwr raises UnsupportedFeature immediately
so the assertion and ws_disconnect() are never reached; fix by calling await
test_charger_v2.update() outside the pytest.raises context, then use with
pytest.raises(UnsupportedFeature): await
test_charger_v2.set_shaper_live_pwr(210), and after that assert "Feature not
supported for older firmware." in caplog.text and finally call await
test_charger_v2.ws_disconnect() to ensure cleanup; keep references to
UnsupportedFeature, set_shaper_live_pwr, update, ws_disconnect, and caplog to
locate the code.
---
Nitpick comments:
In `@tests/test_main.py`:
- Around line 1293-1295: The zero-power test block is missing the assertion that
the response log contains the expected response message; after calling
test_charger.set_shaper_live_pwr(0) and asserting the posting log, add an
assertion that caplog.text includes the response log string used elsewhere (the
same response-check pattern as the 210W case) to verify the response was logged;
locate the zero-power block around the call to test_charger.set_shaper_live_pwr
and mirror the response assertion used with the 210 case to keep tests
consistent.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
1 participant
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.
fixes #515
related to firstof9/openevse#540
Summary by CodeRabbit
New Features
Tests
Chores