fix: price unit API support, ChargeState nullable fields, and legacy model cleanup#101
Merged
Veldkornet merged 3 commits intoJun 17, 2026
Conversation
…cleanup - feat(models): add per_unit field to Price, parsed from API perUnit field (closes HiDiHo01#99) - fix(models): make ChargeState fields nullable to handle wall chargers without a connected EV - refactor(models): remove Old_PriceData, dead helper methods, and legacy duplicate implementations (closes HiDiHo01#100) - refactor(models): migrate Resolution enum from (str, Enum) to StrEnum - fix(models): simplify auth token checks and error handling (ruff SIM102, SIM103, SIM108) - fix(auth): inline JWT-structure guard in is_expired (ruff SIM103) - fix(frank_energie): remove extraneous f-string prefix (ruff F541) - test(models): add regression tests for nullable ChargeState fields - test: fix import ordering and useless-expression violations (ruff E402, B018) - test: merge nested with-statements (ruff SIM117)
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Veldkornet, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
Warning Review limit reached
More reviews will be available in 39 minutes and 21 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough
ChangesAPI client and model overhaul
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
python_frank_energie/frank_energie.py (1)
696-701:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocstring/behavior mismatch: method returns
{}but docstring claims it raisesAuthRequiredException.The test confirms returning
{}is the intended behavior when not authenticated, but the docstring at lines 697-698 still documents raisingAuthRequiredException.📝 Suggested docstring fix
Returns: The enode charger information. - Raises: - AuthRequiredException: If the client is not authenticated. - FrankEnergieException: If the request fails. + Returns an empty dict if the client is not authenticated. + + Raises: + FrankEnergieException: If the request fails. """🤖 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 `@python_frank_energie/frank_energie.py` around lines 696 - 701, The docstring for this method incorrectly documents that AuthRequiredException is raised when the client is not authenticated (lines 697-698), but the actual code returns an empty dictionary instead. Remove the AuthRequiredException from the Raises section of the docstring to align with the actual behavior that returns {} when not authenticated. If needed, document the empty dictionary return behavior in the Returns section instead.python_frank_energie/models.py (2)
3347-3359:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
all_avgshould returnPriceDataAvgsoper_unitworks on aggregate output.At Line 3347, returning a dynamic
type(...)object bypasses the newPriceDataAvg.per_unitproperty, so consumers ofall_avgwon’t see the propagated unit metadata.💡 Suggested fix
@@ - return type( - "PriceDataAvg", - (object,), - { - "values": all_prices, - "total": avg, - "market_price_with_tax_and_markup": market_price_with_tax_and_markup_avg, - "market_markup_price": market_price_markup_avg, - "market_price_with_tax": market_price_with_tax_avg, - "market_price_tax": market_price_tax_avg, - "market_price": market_price_avg, - }, - ) + return PriceDataAvg( + values=all_prices, + total=avg, + market_price_with_tax_and_markup=market_price_with_tax_and_markup_avg, + market_markup_price=market_price_markup_avg, + market_price_with_tax=market_price_with_tax_avg, + market_price_tax=market_price_tax_avg, + market_price=market_price_avg, + )🤖 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 `@python_frank_energie/models.py` around lines 3347 - 3359, The all_avg method is currently returning a dynamically created type object via type(...) instead of a proper PriceDataAvg instance, which prevents consumers from accessing the per_unit property. Replace the dynamic type creation with an instantiation of the PriceDataAvg class, passing the price data fields (all_prices, avg, market_price_with_tax_and_markup_avg, market_price_markup_avg, market_price_with_tax_avg, market_price_tax_avg, market_price_avg) as constructor arguments or attributes to the PriceDataAvg class so that the per_unit property becomes accessible on the returned aggregate output.
4214-4239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden
SmartBatterySessions.from_dictagainst malformed payloads and non-numeric values.At Line 4219 and Line 4229, the code assumes
payload["smartBatterySessions"]is dict-like and can crash withAttributeErrorif the API returns an unexpected shape. At Line 4226,_safe_floatstill raises on non-numeric strings, so it is not actually safe.💡 Suggested fix
@@ - payload = data.get("data") - if not payload: + payload = data.get("data") + if not payload: # return None raise RequestException("Unexpected response") + if not isinstance(payload, Mapping): + raise RequestException("Missing 'data' in SmartBatterySessions response") smart_battery_session_data = payload.get("smartBatterySessions") + if not isinstance(smart_battery_session_data, Mapping): + raise RequestException("Missing 'smartBatterySessions' in response") @@ def _safe_float(val: Any) -> float | None: if val is None or val == "": return None - return float(val) + try: + return float(val) + except (TypeError, ValueError): + return None🤖 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 `@python_frank_energie/models.py` around lines 4214 - 4239, The SmartBatterySessions.from_dict method lacks proper validation of the API response structure. After retrieving smart_battery_session_data from the payload using payload.get("smartBatterySessions"), add a check to ensure it is actually a dictionary and is not None before attempting to call .get() methods on it; raise RequestException if it is malformed. Additionally, the _safe_float helper function is not truly safe because it does not handle exceptions when float(val) fails on non-numeric strings. Wrap the float(val) conversion in a try-except block to catch ValueError exceptions and return None for any values that cannot be converted to float, ensuring the function is actually safe for all non-numeric string inputs.tests/test_frank_energie.py (1)
1159-1181:⚠️ Potential issue | 🟠 MajorRestore the original Windows event-loop policy in teardown instead of unconditionally deleting it.
The test sets
asyncio.WindowsSelectorEventLoopPolicyto a mock (line 1165) but the teardown unconditionally deletes the attribute (lines 1180-1181). On Windows, this removes the real stdlib symbol and breaks test isolation—subsequent tests in the same process will fail to access the real event loop policy.Proposed fix
import asyncio import sys from unittest.mock import MagicMock original_platform = sys.platform + original_policy_class = getattr(asyncio, "WindowsSelectorEventLoopPolicy", None) mock_policy_class = MagicMock() mock_policy = MagicMock() mock_policy_class.return_value = mock_policy @@ finally: sys.platform = original_platform - if hasattr(asyncio, "WindowsSelectorEventLoopPolicy"): + if original_policy_class is None and hasattr(asyncio, "WindowsSelectorEventLoopPolicy"): del asyncio.WindowsSelectorEventLoopPolicy + elif original_policy_class is not None: + asyncio.WindowsSelectorEventLoopPolicy = original_policy_class🤖 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_frank_energie.py` around lines 1159 - 1181, The test unconditionally deletes asyncio.WindowsSelectorEventLoopPolicy in the finally block, which removes the real stdlib symbol on Windows systems and breaks test isolation. Instead of unconditionally deleting the attribute in the teardown, save the original value of asyncio.WindowsSelectorEventLoopPolicy before the try block (checking if it exists first), and then restore it in the finally block to its original state or delete it only if it did not exist originally.
🧹 Nitpick comments (1)
python_frank_energie/models.py (1)
2430-2448: ⚡ Quick winReuse
ChargeState.from_dictinEnodeCharger.from_dictto prevent parser drift.This block duplicates parsing logic and has already diverged (e.g.,
charge_time_remainingnormalization behavior differs fromChargeState.from_dict). Delegating to one parser reduces drift risk.♻️ Suggested refactor
@@ - raw_battery_capacity = charge_state_data.get("batteryCapacity") - raw_battery_level = charge_state_data.get("batteryLevel") - raw_charge_limit = charge_state_data.get("chargeLimit") - raw_range = charge_state_data.get("range") - raw_is_fully_charged = charge_state_data.get("isFullyCharged") - - charge_state = ChargeState( - battery_capacity=float(raw_battery_capacity) if raw_battery_capacity is not None else None, - battery_level=int(raw_battery_level) if raw_battery_level is not None else None, - charge_limit=int(raw_charge_limit) if raw_charge_limit is not None else None, - charge_rate=float(charge_state_data["chargeRate"]) if charge_state_data.get("chargeRate") is not None else None, - charge_time_remaining=charge_state_data.get("chargeTimeRemaining"), - is_charging=bool(charge_state_data["isCharging"]), - is_fully_charged=bool(raw_is_fully_charged) if raw_is_fully_charged is not None else None, - is_plugged_in=bool(charge_state_data["isPluggedIn"]), - last_updated=_parse_iso_datetime(charge_state_data.get("lastUpdated")), - power_delivery_state=charge_state_data["powerDeliveryState"], - range=int(raw_range) if raw_range is not None else None, - ) + charge_state = ChargeState.from_dict(charge_state_data)🤖 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 `@python_frank_energie/models.py` around lines 2430 - 2448, The parsing logic in the EnodeCharger.from_dict method is duplicating the ChargeState.from_dict logic, which has already caused inconsistencies (such as differing behavior for charge_time_remaining normalization). Replace the entire inline ChargeState construction block that manually extracts and converts fields like battery_capacity, battery_level, charge_limit, charge_rate, charge_time_remaining, is_charging, is_fully_charged, is_plugged_in, last_updated, power_delivery_state, and range with a single delegation to ChargeState.from_dict(charge_state_data) to ensure consistent parsing behavior and eliminate future drift between the two parsers.
🤖 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.
Inline comments:
In `@python_frank_energie/frank_energie.py`:
- Around line 2333-2334: Replace the Dutch error message "Authenticatie is
vereist." in the AuthRequiredException raised when self.is_authenticated is
false with the English equivalent "Authentication is required." to maintain
consistency with the rest of the codebase where this same English message is
used throughout the file.
- Around line 2896-2914: The introspect_schema method incorrectly uses
requests.post() with a context manager (with statement), but requests.Response
does not implement the context manager protocol (__enter__/__exit__ methods),
which will cause an AttributeError at runtime. Remove the with statement and
instead directly assign the result of requests.post() to the response variable,
then call response.raise_for_status() and return response.json() on the assigned
variable as normal.
In `@python_frank_energie/models.py`:
- Around line 4690-4705: The from_dict classmethod in UserSmartFeedInStatus
silently maps GraphQL failures to None by only checking if the payload is None,
making it impossible to distinguish between a legitimate "no feed-in contract"
response (where userSmartFeedIn is explicitly null) and actual API errors or
malformed responses. Before extracting the userSmartFeedIn payload, check the
GraphQL response for an errors field; if errors are present, raise an
appropriate exception rather than returning None. Only return None when
userSmartFeedIn is explicitly null in a valid, error-free response.
In `@tests/conftest.py`:
- Around line 21-29: The force_enable_socket fixture at lines 21-29 uses the
private internal attribute pytest_socket._true_socket and mutates the global
socket.socket in an autouse fixture, causing cross-test state leakage and
fragile coupling to pytest-socket internals. Since the file already calls the
public pytest_socket.enable_socket() method at other locations and tests use the
proper `@pytest.mark.allow_socket` marker, this fixture is redundant. Either
remove the entire force_enable_socket fixture completely, or if it must be
retained, replace the private API usage by calling the public
pytest_socket.enable_socket() method instead of directly assigning
socket.socket.
---
Outside diff comments:
In `@python_frank_energie/frank_energie.py`:
- Around line 696-701: The docstring for this method incorrectly documents that
AuthRequiredException is raised when the client is not authenticated (lines
697-698), but the actual code returns an empty dictionary instead. Remove the
AuthRequiredException from the Raises section of the docstring to align with the
actual behavior that returns {} when not authenticated. If needed, document the
empty dictionary return behavior in the Returns section instead.
In `@python_frank_energie/models.py`:
- Around line 3347-3359: The all_avg method is currently returning a dynamically
created type object via type(...) instead of a proper PriceDataAvg instance,
which prevents consumers from accessing the per_unit property. Replace the
dynamic type creation with an instantiation of the PriceDataAvg class, passing
the price data fields (all_prices, avg, market_price_with_tax_and_markup_avg,
market_price_markup_avg, market_price_with_tax_avg, market_price_tax_avg,
market_price_avg) as constructor arguments or attributes to the PriceDataAvg
class so that the per_unit property becomes accessible on the returned aggregate
output.
- Around line 4214-4239: The SmartBatterySessions.from_dict method lacks proper
validation of the API response structure. After retrieving
smart_battery_session_data from the payload using
payload.get("smartBatterySessions"), add a check to ensure it is actually a
dictionary and is not None before attempting to call .get() methods on it; raise
RequestException if it is malformed. Additionally, the _safe_float helper
function is not truly safe because it does not handle exceptions when float(val)
fails on non-numeric strings. Wrap the float(val) conversion in a try-except
block to catch ValueError exceptions and return None for any values that cannot
be converted to float, ensuring the function is actually safe for all
non-numeric string inputs.
In `@tests/test_frank_energie.py`:
- Around line 1159-1181: The test unconditionally deletes
asyncio.WindowsSelectorEventLoopPolicy in the finally block, which removes the
real stdlib symbol on Windows systems and breaks test isolation. Instead of
unconditionally deleting the attribute in the teardown, save the original value
of asyncio.WindowsSelectorEventLoopPolicy before the try block (checking if it
exists first), and then restore it in the finally block to its original state or
delete it only if it did not exist originally.
---
Nitpick comments:
In `@python_frank_energie/models.py`:
- Around line 2430-2448: The parsing logic in the EnodeCharger.from_dict method
is duplicating the ChargeState.from_dict logic, which has already caused
inconsistencies (such as differing behavior for charge_time_remaining
normalization). Replace the entire inline ChargeState construction block that
manually extracts and converts fields like battery_capacity, battery_level,
charge_limit, charge_rate, charge_time_remaining, is_charging, is_fully_charged,
is_plugged_in, last_updated, power_delivery_state, and range with a single
delegation to ChargeState.from_dict(charge_state_data) to ensure consistent
parsing behavior and eliminate future drift between the two parsers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc5caf85-c7b3-4947-8c96-8016ef72205b
📒 Files selected for processing (15)
python_frank_energie/authentication.pypython_frank_energie/frank_energie.pypython_frank_energie/models.pytests/__snapshots__/test_frank_energie.ambrtests/__snapshots__/test_models.ambrtests/__snapshots__/test_smart_batteries.ambrtests/conftest.pytests/fixtures/invoices.jsontests/fixtures/market_prices.jsontests/fixtures/month_summary.jsontests/fixtures/smart_battery_sessions.jsontests/test_contract_price_resolution_mutations.pytests/test_frank_energie.pytests/test_models.pytests/test_renew_token.py
|
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
2 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.
Summary
This PR addresses three related issues in the library layer: API-provided gas unit support, robustness fixes for EV charger data, and removal of legacy/duplicate model code.
Key Changes
KWH,M3) and exposes it asprice.per_unitso the HA integration can select the correct unit without hardcoding itbattery_capacity,battery_level,charge_limit,charge_rate,charge_time_remaining,is_plugged_in,power_delivery_stateare now| NonewithNoneguards infrom_dict, preventing a silentTypeErrorwhen a wall charger has no connected EVOld_PriceData— legacy class, dead helper methods (old_avg,older_avg,old_elec_previoushour,old_elec_nexthour), and unusedPriceData.from_dict()removed, leaving one canonical implementationResolutionmigrated from(str, Enum)toStrEnum; SIM102/SIM103/SIM108/SIM117/D401/E402/B018/F541 violations resolved across all touched filesChargeState; snapshot and fixture files updated to reflect model changesWhy This Is Needed
The
perUnitfield is the only reliable way to distinguish Dutch (kWh) from Belgian (GJ) gas contracts at runtime. Without it, the HA integration was forced to hardcode the unit, which broke Belgian users.The
ChargeStateTypeErrorwas silently swallowing charger data for users whose EV was not connected, making all charger sensors unavailable.The
Old_PriceDatacleanup reduces maintenance surface and eliminates a potential double-conversion bug wherePriceobjects were passed to a constructor expectinglist[dict].Closes
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation