feat: support vehicle range unit of measure and configuration#628
Conversation
📝 WalkthroughWalkthroughAdds a ChangesMQTT Vehicle Range Miles Feature
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 docstrings
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_properties.py (1)
495-508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one assertion for the
battery_rangefallback path.This test validates unit switching well, but it doesn’t cover the legacy fallback key (
battery_range) invehicle_range. Adding that case would protect the backward payload contract.🤖 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_properties.py` around lines 495 - 508, The test_mqtt_vehicle_range_miles function validates unit switching for vehicle_range but does not cover the legacy fallback path where battery_range is used instead of vehicle_range. Add an assertion after the existing vehicle_range assertions that sets charger._status with battery_range (instead of vehicle_range) and verifies that charger.vehicle_range still returns the correct unit conversion based on the mqtt_vehicle_range_miles configuration. This will ensure the backward compatibility fallback is working correctly.
🤖 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 `@openevsehttp/properties.py`:
- Around line 459-468: The vehicle_range method has changed its return type from
numeric to tuple[int, str] | None, which breaks the existing API contract and
will cause runtime errors in code expecting numeric values. To maintain backward
compatibility, keep the vehicle_range method returning only the numeric value as
before, and create a new separate property (such as vehicle_range_with_unit)
that returns the tuple containing both the value and unit string. This way
existing consumers of vehicle_range will continue to work while new code can
opt-in to using the unit-aware version.
In `@tests/test_commands.py`:
- Around line 1360-1361: In the pytest.raises assertion for the
set_mqtt_vehicle_range_miles method call, the match parameter contains an
unescaped period (.) in "Value must be a boolean." which acts as a regex
wildcard matching any character instead of a literal period. Escape the period
by replacing it with \. so the regex pattern only matches the exact error
message string.
---
Nitpick comments:
In `@tests/test_properties.py`:
- Around line 495-508: The test_mqtt_vehicle_range_miles function validates unit
switching for vehicle_range but does not cover the legacy fallback path where
battery_range is used instead of vehicle_range. Add an assertion after the
existing vehicle_range assertions that sets charger._status with battery_range
(instead of vehicle_range) and verifies that charger.vehicle_range still returns
the correct unit conversion based on the mqtt_vehicle_range_miles configuration.
This will ensure the backward compatibility fallback is working correctly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9b21444-7377-4913-98fc-2453483266bf
📒 Files selected for processing (4)
openevsehttp/commands.pyopenevsehttp/properties.pytests/test_commands.pytests/test_properties.py
c1d557f to
9a91746
Compare
This comment was marked as outdated.
This comment was marked as outdated.
9a91746 to
b824160
Compare
b824160 to
440daeb
Compare
Code Review for PR #628 — support vehicle range unit of measure and configuration✅ Looks Good
💡 Suggestions
Approved. Ready to merge. |
Description
This PR adds a new
vehicle_range_with_unitproperty that returns a tuple containing the range value and the unit of measure ("miles"or"km"), depending on the configuration value ofmqtt_vehicle_range_miles. It keeps thevehicle_rangeproperty returning only the numeric value to maintain backward compatibility. It also adds themqtt_vehicle_range_milesproperty to retrieve this setting and theset_mqtt_vehicle_range_miles(self, enable: bool)command method to configure this setting via the POST/configAPI endpoint.Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist
Summary by CodeRabbit
Release Notes
New Features
Deprecation
vehicle_rangeproperty is deprecated; use the new property returning range with unit instead.