Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1352 +/- ##
=======================================
Coverage 99.88% 99.89%
=======================================
Files 6 6
Lines 906 910 +4
Branches 126 126
=======================================
+ Hits 905 909 +4
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request enhances type safety for the EntityInfo model by making it generic and improving handling of undefined values from BSB-LAN devices. The PR converts the string "---" (used by BSB-LAN for inactive sensors) to None consistently, and introduces a generic type parameter to EntityInfo to indicate expected value types after conversion.
Changes:
- Made
EntityInfoa generic class with type parameterTto indicate expected value types (float, int, str, or time) - Updated
"---"handling to convert toNoneinstead of preserving the string - Added
EntityValuetype alias and exposed it in the public API
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/bsblan/models.py | Made EntityInfo generic, added EntityValue type alias, updated conversion logic to handle "---" as None |
| src/bsblan/bsblan.py | Added type: ignore comments for min/max temp assignments, changed logging format from %f to %s |
| src/bsblan/init.py | Exposed new EntityValue type in public API |
| tests/test_entity_info.py | Added test for "---" to None conversion, updated existing tests for new behavior |
| tests/test_state.py | Updated test assertion to expect None instead of "---" string |
| pyproject.toml | Disabled UP046 linting rule for compatibility |
|
|
||
| @dataclass | ||
| class EntityInfo(DataClassJSONMixin): | ||
| class EntityInfo(DataClassJSONMixin, Generic[T]): |
There was a problem hiding this comment.
The EntityInfo class is made generic with type parameter T, but it's never actually used with explicit type parameters anywhere in the codebase (e.g., EntityInfo[float], EntityInfo[int]). All field declarations use plain EntityInfo | None.
This means the generic type parameter provides no actual type safety benefit - the type checker cannot verify that the value is of the expected type. The cast to T at line 321 is unchecked and could be incorrect.
Either:
- Use explicit type parameters in model field declarations (e.g., min_temp: EntityInfo[float] | None), OR
- Remove the generic implementation and keep value typed as the union EntityValue | None directly
The current implementation gives a false sense of type safety without actually providing it.
| result = int(raw) | ||
|
|
||
| return result | ||
| return cast("T", result) |
There was a problem hiding this comment.
The cast to T here is unsafe because the actual type of result may not match T. For example:
- If data_type is PLAIN_NUMBER but the value is not a temperature and has no decimal point, result will be int
- If data_type is PLAIN_NUMBER and the value is a temperature, result will be float
- If conversion fails and suppress catches the exception, result remains as str
Since EntityInfo is not instantiated with explicit type parameters, T is essentially unconstrained here, making this cast to "T" (with quotes) essentially a lie to the type checker.
Consider either using a proper TypeGuard pattern to validate the type, or removing the generic implementation entirely and returning EntityValue | None directly.
| def __post_init__(self) -> None: | ||
| """Convert values based on data_type after initialization.""" | ||
| if self.value == "---": # Special case for undefined values | ||
| if self.value == "---": # Sensor/parameter not in use |
There was a problem hiding this comment.
The check at line 267 only handles the string "---" but not the case where the value field might be null in the JSON (which would deserialize to None directly). If the BSB-LAN API ever returns "value": null instead of "value": "---", this would bypass the None-setting logic and cause str(None) to be called at line 291, resulting in the string "None" being converted.
Consider also checking for None explicitly:
if self.value == "---" or self.value is None:
self.value = None
return| if self.value == "---": # Sensor/parameter not in use | |
| if self.value == "---" or self.value is None: | |
| # Sensor/parameter not in use |
| "D213", # Conflicts with other rules | ||
| "D417", # False positives in some occasions | ||
| "PLR2004", # Just annoying, not really useful | ||
| "UP046", # Generic subclass style kept for mashumaro compatibility |
There was a problem hiding this comment.
The comment for ignoring UP046 mentions "Generic subclass style kept for mashumaro compatibility" but UP046 is actually about type alias declarations, not generic subclass syntax. The rule suggests using type EntityValue = float | int | str | time (PEP 695) or EntityValue: TypeAlias = ... instead of plain assignment.
The comment should be clarified to accurately describe why UP046 is being ignored. If it's genuinely for mashumaro compatibility, please explain how the type alias syntax affects mashumaro. Otherwise, consider updating to use the modern type alias syntax.
| "UP046", # Generic subclass style kept for mashumaro compatibility | |
| "UP046", # Type alias style kept for mashumaro compatibility |
| self._min_temp = static_values.min_temp.value # type: ignore[assignment] | ||
| logger.debug("Min temperature initialized: %s", self._min_temp) |
There was a problem hiding this comment.
The type: ignore comment is suppressing a real type safety issue. The value from static_values.min_temp.value could be None (if the sensor returns "---") or could be a string (if conversion failed). This should be handled explicitly with a type check and appropriate error handling rather than using type: ignore.
Consider checking if the value is a float before assignment:
if isinstance(static_values.min_temp.value, float):
self._min_temp = static_values.min_temp.value
logger.debug("Min temperature initialized: %s", self._min_temp)
else:
logger.warning("min_temp value is not a float: %s", static_values.min_temp.value)| self._max_temp = static_values.max_temp.value # type: ignore[assignment] | ||
| logger.debug("Max temperature initialized: %s", self._max_temp) |
There was a problem hiding this comment.
The type: ignore comment is suppressing a real type safety issue. The value from static_values.max_temp.value could be None (if the sensor returns "---") or could be a string (if conversion failed). This should be handled explicitly with a type check and appropriate error handling rather than using type: ignore.
Consider checking if the value is a float before assignment:
if isinstance(static_values.max_temp.value, float):
self._max_temp = static_values.max_temp.value
logger.debug("Max temperature initialized: %s", self._max_temp)
else:
logger.warning("max_temp value is not a float: %s", static_values.max_temp.value)


This pull request introduces type safety and improved handling of undefined values for the
EntityInfomodel, along with related updates across the codebase and tests. The main focus is on makingEntityInfogeneric over its value type, ensuring that values are properly converted and that special cases (like"---"for inactive sensors) are handled consistently asNone.EntityInfo type safety and conversion improvements:
EntityInfoa generic class (EntityInfo[T]) with a newEntityValuetype union, ensuring that thevaluefield is always of the correct type after conversion (float, int, str, or time) orNoneif undefined. (src/bsblan/models.py) [1] [2] [3]EntityInfoto setvaluetoNonewhen the device returns"---"and to perform type-checked conversion based ondata_type. (src/bsblan/models.py) [1] [2]Public API and import updates:
EntityValuetype and updatedEntityInfoin the public API (__init__.py). (src/bsblan/__init__.py) [1] [2]Test updates for new behavior:
"---"values are converted toNone, and that conversion errors preserve the original value and are logged. (tests/test_entity_info.py,tests/test_state.py) [1] [2] [3]Internal usage and logging:
EntityInfovalue typing, including logging and assignment of min/max temperature values without forced float conversion. (src/bsblan/bsblan.py) [1] [2]Linting configuration:
UP046linting rule to maintain compatibility withmashumaro's generic subclass style. (pyproject.toml)