Skip to content

feat: support vehicle range unit of measure and configuration#628

Merged
firstof9 merged 1 commit into
mainfrom
feature/vehicle-range-units-miles
Jun 23, 2026
Merged

feat: support vehicle range unit of measure and configuration#628
firstof9 merged 1 commit into
mainfrom
feature/vehicle-range-units-miles

Conversation

@firstof9

@firstof9 firstof9 commented Jun 23, 2026

Copy link
Copy Markdown
Owner

Description

This PR adds a new vehicle_range_with_unit property that returns a tuple containing the range value and the unit of measure ("miles" or "km"), depending on the configuration value of mqtt_vehicle_range_miles. It keeps the vehicle_range property returning only the numeric value to maintain backward compatibility. It also adds the mqtt_vehicle_range_miles property to retrieve this setting and the set_mqtt_vehicle_range_miles(self, enable: bool) command method to configure this setting via the POST /config API endpoint.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code quality / Refactoring
  • Documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My code is self-documenting and does not require comments
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Summary by CodeRabbit

Release Notes

  • New Features

    • Added configuration option to set vehicle range display units (miles or kilometers) via MQTT.
    • Added new property returning vehicle range with its corresponding unit.
  • Deprecation

    • vehicle_range property is deprecated; use the new property returning range with unit instead.

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a mqtt_vehicle_range_miles boolean config flag and a set_mqtt_vehicle_range_miles command that POSTs it to the charger. The existing vehicle_range property is deprecated in favor of a new vehicle_range_with_unit property that returns a (value, unit) tuple where the unit is "miles" or "km" based on the flag.

Changes

MQTT Vehicle Range Miles Feature

Layer / File(s) Summary
vehicle_range deprecation, vehicle_range_with_unit, mqtt_vehicle_range_miles, and set command
openevsehttp/properties.py, openevsehttp/commands.py
warnings imported; vehicle_range emits DeprecationWarning; new vehicle_range_with_unit returns (value, "miles"|"km") based on mqtt_vehicle_range_miles config flag; mqtt_vehicle_range_miles reads boolean from _config (default False); set_mqtt_vehicle_range_miles validates bool input and POSTs the config update, raising UnknownError on failure.
Command and property tests
tests/test_commands.py, tests/test_properties.py
test_set_mqtt_vehicle_range_miles covers success for True/False, TypeError on non-boolean input, and UnknownError on error response; parametrized property table updated to expect (468, "km") tuple for vehicle_range_with_unit; test_simple_properties wraps vehicle_range assertions in pytest.deprecated_call(); new test_mqtt_vehicle_range_miles asserts unit selection and deprecated scalar value under each config state.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 Hop hop, the range now speaks in miles or km true,
A tuple returned with unit — no more guessing what to do!
The old vehicle_range whispers "deprecated, friend,"
While vehicle_range_with_unit rides the data trend.
POSTs the config, checks the flag, raises errors right,
This bunny's code review leaps with pure delight! 🌟

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the main feature being added: support for vehicle range unit of measure and configuration.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description covers the main changes, type of change, and a completed checklist, but lacks specific issue reference and detailed motivation/context.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@firstof9 firstof9 changed the title Support vehicle range unit of measure and configuration feat: support vehicle range unit of measure and configuration Jun 23, 2026
@github-actions github-actions Bot added the feature New Features label Jun 23, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
tests/test_properties.py (1)

495-508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add one assertion for the battery_range fallback path.

This test validates unit switching well, but it doesn’t cover the legacy fallback key (battery_range) in vehicle_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

📥 Commits

Reviewing files that changed from the base of the PR and between 6bc6848 and c1d557f.

📒 Files selected for processing (4)
  • openevsehttp/commands.py
  • openevsehttp/properties.py
  • tests/test_commands.py
  • tests/test_properties.py

Comment thread openevsehttp/properties.py Outdated
Comment thread tests/test_commands.py Outdated
@firstof9 firstof9 force-pushed the feature/vehicle-range-units-miles branch from c1d557f to 9a91746 Compare June 23, 2026 15:18
@secondof9

This comment was marked as outdated.

@firstof9 firstof9 force-pushed the feature/vehicle-range-units-miles branch from 9a91746 to b824160 Compare June 23, 2026 15:31
@firstof9 firstof9 force-pushed the feature/vehicle-range-units-miles branch from b824160 to 440daeb Compare June 23, 2026 15:58
@secondof9

Copy link
Copy Markdown

Code Review for PR #628 — support vehicle range unit of measure and configuration

✅ Looks Good

  • Correctness — The PR correctly extends the OpenEVSE client with a new vehicle_range_with_unit property that returns a tuple of (range, unit), while preserving the existing vehicle_range property (now deprecated via DeprecationWarning with stacklevel=2). The mqtt_vehicle_range_miles config property and set_mqtt_vehicle_range_miles() command method are fully implemented with proper type validation, logging, and error handling. The diff cleanly adds 103 lines (20 in commands, 23 in properties, 33 in commands tests, 27 in properties tests) with zero deletions in the source code.

  • Edge cases — The vehicle_range_with_unit handles None values gracefully, returning None when the API reports no range. The mqtt_vehicle_range_miles property defaults to False when absent from the config, matching the spec. The set_mqtt_vehicle_range_miles() method validates the enable argument for bool, raising TypeError with a clear message when a non-boolean is provided. Failed server responses surface UnknownError via the existing error-handling path.

  • Security — The config mutation sends JSON with a single boolean field to POST /config using the existing process_request() mechanism. No secrets or injection risks introduced.

  • Quality — Docstrings explain the behavior and cross-property effects. All four changed files are modified with minimal, targeted deltas. The deprecation warning on vehicle_range gives callers a clear migration path.

  • Testing — Tests cover the happy path (True/False), type error with string argument, and failed server responses (400-like body). Property tests also verify unit selection (miles vs km).

💡 Suggestions

  • Consider adding a docstring note to vehicle_range_with_unit that it requires VehicleRange to be enabled on the charger (since the unit is derived from the mqtt_vehicle_range_miles MQTT config field).

Approved. Ready to merge.

@firstof9 firstof9 merged commit 1f850ad into main Jun 23, 2026
17 checks passed
@firstof9 firstof9 deleted the feature/vehicle-range-units-miles branch June 23, 2026 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New Features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants