refactor: optimize properties to return None if value missing
#504
Conversation
📝 WalkthroughWalkthroughMultiple OpenEVSE property getters were hardened: type hints broadened to Optional, direct dict indexing replaced with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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)openevsehttp/__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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openevsehttp/__main__.py (2)
1162-1168: Inconsistent pattern: assertion and direct indexing remain.This property still uses
assert self._config is not Noneand direct dictionary indexingself._config["protocol"], which is inconsistent with the PR's goal of graceful handling. If theprotocolkey is missing, this will raiseKeyError.Proposed fix
`@property` def protocol_version(self) -> str | None: """Return the protocol version.""" - assert self._config is not None - if self._config["protocol"] == "-": + protocol = self._config.get("protocol") + if protocol is None or protocol == "-": return None - return self._config["protocol"] + return protocol
1223-1233: Logic error: should useall()instead ofany().The condition checks if any of
voltageorampexists, but both are required for the calculation. If only one key exists, the direct indexing on line 1232 will raiseKeyError.Proposed fix
`@property` def charging_power(self) -> float | None: """Return the charge power. Calculate Watts base on V*I """ - if self._status is not None and any( + if self._status is not None and all( key in self._status for key in ["voltage", "amp"] ): return round(self._status["voltage"] * self._status["amp"], 2) return None
🤖 Fix all issues with AI agents
In `@openevsehttp/__main__.py`:
- Around line 898-904: The led_brightness property's return type annotation is
wrong: update the annotation on the led_brightness property from "str | None" to
"int | None" and ensure callers still read the value from
self._config.get("led_brightness") (no other logic change); verify
_version_check("4.1.0") and the UnsupportedFeature raise remain intact and that
any downstream uses expect an int or None.
- Around line 1127-1135: The usage_session property risks a KeyError by indexing
self._status["wattsec"]; update usage_session to safely access keys (use
self._status.get("session_energy") first, then wattsec =
self._status.get("wattsec")) and if wattsec is None return None, otherwise
compute and return float(round(wattsec / 3600, 2)); reference the usage_session
property and the "session_energy" and "wattsec" keys when making the change.
- Around line 951-954: The service_level property is annotated as str | None but
tests expect an int | None; update the annotation to int | None and convert the
stored value from self._config.get("service") to an int before returning (e.g.,
fetch value = self._config.get("service"); if value is None return None;
otherwise return int(value)), and guard the conversion with a try/except
ValueError to return None (or handle/log) if the config contains a non-integer
string; reference the service_level property and the use of
self._config.get("service") when making the change.
🧹 Nitpick comments (1)
openevsehttp/__main__.py (1)
1079-1085: Consider simplifying the key check (static analysis hint).The
"temp" in self._statuscheck before using.get()is flagged by Ruff (RUF019). While the current logic works, you could simplify by relying on.get()with a falsy check:`@property` def ambient_temperature(self) -> float | None: """Return the temperature of the ambient sensor, in degrees Celsius.""" - if "temp" in self._status and self._status["temp"]: - return self._status.get("temp", 0) / 10 - else: - return self._status.get("temp1", 0) / 10 + temp = self._status.get("temp") + if temp: + return temp / 10 + return self._status.get("temp1", 0) / 10Note: If both
tempandtemp1are missing, this returns0.0rather thanNone. Consider whether returningNonewould be more consistent with other temperature properties.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openevsehttp/__main__.py`:
- Around line 1035-1043: The status() method and state property call
int(self._status.get("state", 0)) which will raise on None or non-numeric
strings; update both to safely coerce state by retrieving raw =
self._status.get("state", 0) then attempting to parse to int with a try/except
(or use str.isdigit-like check) and fall back to 0 on TypeError/ValueError, then
use that safe_index when indexing states in status() and state property
(referencing the status() method, state property, and self._status).
- Around line 1080-1084: The ambient_temperature method currently defaults
missing sensor values to 0; change it to return None when the ambient
temperature is absent instead of using 0. Update ambient_temperature to first
check for the presence of the "temp" key in self._status (use key-in-dict, not
truthiness) and if present and not None return self._status["temp"] / 10;
otherwise check "temp1" similarly and return its value/10 if present, and return
None if neither key exists or both are None. This preserves 0 as a valid reading
and avoids defaulting to 0 when sensors are missing.
♻️ Duplicate comments (1)
openevsehttp/__main__.py (1)
952-954: Service level should be numeric (notstr | None).Returning a string here is inconsistent with the API/tests and can break callers expecting an int. Consider coercing to
int | Nonewith a safe fallback.💡 Proposed fix
`@property` -def service_level(self) -> str | None: +def service_level(self) -> int | None: """Return the service level.""" - return self._config.get("service") + value = self._config.get("service") + if value is None: + return None + try: + return int(value) + except (TypeError, ValueError): + _LOGGER.debug("Invalid service level: %s", value) + return None
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openevsehttp/__main__.py`:
- Around line 1088-1114: The three accessors rtc_temperature, ir_temperature,
and esp_temperature currently treat falsy 0 as missing and will raise on
non-numeric strings; update each to explicitly check for None and the string
"false" (i.e., if temp is None or temp == "false" return None), then attempt to
convert temp to a numeric (int/float) inside a try/except (or use a safe parse)
and on success return numeric_value / 10, otherwise return None if parsing
fails; ensure esp_temperature still handles the absent "temp4" key the same way
before parsing.
♻️ Duplicate comments (3)
openevsehttp/__main__.py (3)
951-960:service_levelshould beint | Nonewith safe parsing.
Same concern as earlier review: the value is numeric but annotated asstr | None.🛠️ Proposed fix
- def service_level(self) -> str | None: + def service_level(self) -> int | None: """Return the service level.""" - return self._config.get("service") + value = self._config.get("service") + if value is None: + return None + try: + return int(value) + except (TypeError, ValueError): + return None
1035-1042: Guardstateparsing to avoidTypeError/ValueError.
Same issue as earlier review:int(self._status.get("state", 0))can fail onNoneor non-numeric strings.🛠️ Proposed fix
def status(self) -> str: """Return charger's state.""" - return self._status.get("status", states[int(self._status.get("state", 0))]) + state = self._status.get("state") + try: + state_int = int(state) if state is not None else 0 + except (TypeError, ValueError): + state_int = 0 + return self._status.get("status", states[state_int]) def state(self) -> str: """Return charger's state.""" - return states[int(self._status.get("state", 0))] + state = self._status.get("state") + try: + state_int = int(state) if state is not None else 0 + except (TypeError, ValueError): + state_int = 0 + return states[state_int]
1073-1085: ReturnNonewhen ambient temperature is missing (don’t coerce to 0).
Same issue as earlier review: current logic treats0as missing and returns0when both keys are absent.🌡️ Proposed fix
def ambient_temperature(self) -> float | None: """Return the temperature of the ambient sensor, in degrees Celsius.""" - temp = self._status.get("temp") - if temp: - return temp / 10 - return self._status.get("temp1", 0) / 10 + temp = self._status.get("temp") + if temp is not None and temp != "false": + return temp / 10 + temp1 = self._status.get("temp1") + if temp1 is not None and temp1 != "false": + return temp1 / 10 + return None
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary by CodeRabbit
Bug Fixes
API
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.