Add flag to suppress time updates by client#1733
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1733 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 26
Lines 4231 4233 +2
=========================================
+ Hits 4231 4233 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
…omeapi into provide-time
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-out flag to prevent the client-side connection from responding to ESPHome GetTimeRequest messages, allowing callers (e.g., log runners) to avoid modifying a device’s time/timezone via API time sync while keeping the default behavior unchanged.
Changes:
- Add
provide_time: bool = TruetoConnectionParamsand plumb it fromAPIClientBase.__init__. - Conditionally register the internal
GetTimeRequesthandler based onprovide_time. - Add a dedicated test module validating default behavior, flag propagation, handler registration, and time response shape.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
aioesphomeapi/client_base.py |
Adds provide_time argument and forwards it into ConnectionParams. |
aioesphomeapi/connection.py |
Adds provide_time to params and gates GetTimeRequest handler registration. |
aioesphomeapi/connection.pxd |
Adds provide_time field to the Cython ConnectionParams declaration. |
tests/test_provide_time.py |
Adds tests covering the new flag behavior and handler registration. |
Comments suppressed due to low confidence (1)
aioesphomeapi/client_base.py:337
- The APIClientBase.init docstring lists parameters through
timezonebut does not document the newly addedprovide_timeargument. Please add a:param provide_time:entry explaining that setting it to False disables handling of GetTimeRequest/time sync responses (default True).
"""Create a client, this object is shared across sessions.
:param address: The address to connect to; for example an IP address
or .local name for mDNS lookup.
:param port: The port to connect to
:param password: Optional password to send to the device for authentication
:param client_info: User Agent string to send.
:param keepalive: The keepalive time in seconds (ping interval) for detecting stale connections.
Every keepalive seconds a ping is sent, if no pong is received the connection is closed.
:param zeroconf_instance: Pass a zeroconf instance to use if an mDNS lookup is necessary.
:param noise_psk: Encryption preshared key for noise transport encrypted sessions.
:param expected_name: Require the devices name to match the given expected name.
Can be used to prevent accidentally connecting to a different device if
IP passed as address but DHCP reassigned IP.
:param addresses: Optional list of IP addresses to connect to which takes
precedence over the address parameter. This is most commonly used when
the device has dual stack IPv4 and IPv6 addresses and you do not know
which one to connect to.
:param expected_mac: Optional MAC address to check against the device.
The format should be lower case without : or - separators.
Example: 00:aa:22:33:44:55 -> 00aa22334455
:param timezone: Optional IANA timezone name to send to ESPHome devices.
If not provided, the system timezone will be detected automatically.
Example: 'America/Chicago' or 'Europe/London'
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a ChangesOptional Time Provision Flag
Sequence Diagram(s)sequenceDiagram
participant APIClient
participant Connection
participant Device
APIClient->>Connection: construct ConnectionParams(provide_time)
Connection->>Connection: if provide_time register GetTimeRequest handler
Device->>Connection: GetTimeRequest
Connection->>Device: GetTimeResponse (epoch_seconds[, timezone])
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
aioesphomeapi/client_base.py (1)
311-337: ⚡ Quick winDocument the new
provide_timekwarg inAPIClientBase.__init__docstring.The constructor now accepts
provide_time, but the parameter block does not describe it. Please add a short:param provide_time:entry for caller-facing clarity.Suggested diff
:param timezone: Optional IANA timezone name to send to ESPHome devices. If not provided, the system timezone will be detected automatically. Example: 'America/Chicago' or 'Europe/London' + :param provide_time: Whether to reply to GetTimeRequest messages. + Set to False to avoid client-driven time/timezone updates on the device. """As per coding guidelines, "Docstrings should be terse and default to single-line format ("""Summary.""") describing what the function does and what the caller can pass."
🤖 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 `@aioesphomeapi/client_base.py` around lines 311 - 337, The __init__ docstring for APIClientBase is missing documentation for the new provide_time parameter; update the APIClientBase.__init__ docstring to add a concise ":param provide_time: bool" entry describing that it controls whether the client advertises/sends the current time to the device (default True) so callers understand its purpose and default behavior and ensure the summary remains terse and follows the file's single-line docstring style.tests/test_provide_time.py (1)
1-10: ⚡ Quick winTrim module prose and separator comments to match repository style rules.
The module docstring is overly explanatory, and section-divider comments are unnecessary here. Keep a terse summary and rely on test names for structure.
Suggested diff
-"""Tests for the provide_time flag on APIClient / ConnectionParams. - -When provide_time=True (the default) the connection registers a handler -for GetTimeRequest and responds with the current epoch time, keeping the -device's clock in sync with the client. - -When provide_time=False the handler is not registered, leaving the -device's clock untouched — useful for ESPHome's own log runner which -should not override timezone settings managed by Home Assistant. -""" +"""Test provide_time behavior on APIClient and APIConnection.""" @@ -# --------------------------------------------------------------------------- -# Tests: APIClient stores the flag correctly on _params -# --------------------------------------------------------------------------- @@ -# --------------------------------------------------------------------------- -# Tests: _register_internal_message_handlers respects the flag -# --------------------------------------------------------------------------- @@ -# --------------------------------------------------------------------------- -# Tests: the time response handler sends a plausible epoch value -# ---------------------------------------------------------------------------As per coding guidelines, "Docstrings should be terse and default to single-line format ("""Summary.""")..." and "Add comments only when the why is non-obvious... Default to writing no comments."
Also applies to: 25-27, 44-47, 85-87
🤖 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_provide_time.py` around lines 1 - 10, The module docstring and several section-divider comments are too verbose; replace the multi-line docstring at the top with a terse single-line summary (e.g., "Tests for the provide_time flag on APIClient/ConnectionParams.") and remove the unnecessary separator or explanatory comments around the provide_time-related tests (the long explanatory blocks referenced in the review). Keep test function names and assertions intact so behavior is unchanged.
🤖 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.
Nitpick comments:
In `@aioesphomeapi/client_base.py`:
- Around line 311-337: The __init__ docstring for APIClientBase is missing
documentation for the new provide_time parameter; update the
APIClientBase.__init__ docstring to add a concise ":param provide_time: bool"
entry describing that it controls whether the client advertises/sends the
current time to the device (default True) so callers understand its purpose and
default behavior and ensure the summary remains terse and follows the file's
single-line docstring style.
In `@tests/test_provide_time.py`:
- Around line 1-10: The module docstring and several section-divider comments
are too verbose; replace the multi-line docstring at the top with a terse
single-line summary (e.g., "Tests for the provide_time flag on
APIClient/ConnectionParams.") and remove the unnecessary separator or
explanatory comments around the provide_time-related tests (the long explanatory
blocks referenced in the review). Keep test function names and assertions intact
so behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3b92d65-3287-4f61-95b8-fb1c9814c8a3
📒 Files selected for processing (4)
aioesphomeapi/client_base.pyaioesphomeapi/connection.pxdaioesphomeapi/connection.pytests/test_provide_time.py
|
Tests are failing. Looks like an update is needed |
PR Review — Add flag to suppress time updates by clientThe production change is correct and minimal: 🟡 Important1. Assertion doesn't verify a GetTimeResponse was actually sent (`tests/test_provide_time.py`, L91-93)
The existing assert transport.writelines.call_count == 1
raw = b"".join(transport.writelines.call_args[0][0])
resp = GetTimeResponse()
resp.ParseFromString(raw[3:]) # skip 0x00, length varuint, type varuintMirror that here. For the 🟢 Suggestions1. Use a normal top-level `patch` import instead of dynamic `__import__` (`tests/test_provide_time.py`, L66-71)
Checklist
SummaryThe production change is correct and minimal: To rebase specific severity levels, mention me: Automated review by Kōan2307017 |
…omeapi into provide-time
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_provide_time.py (1)
1-5: ⚡ Quick winCondense the module docstring to a single line.
The multi-line module docstring should be simplified to match project conventions. As per coding guidelines, docstrings should be terse and default to single-line format.
♻️ Proposed simplification
-"""Tests for the provide_time flag on APIClient / ConnectionParams. - -When provide_time=True (the default) the connection registers a handler -device's clock in sync with the client, otherwise ignores time requests. -""" +"""Tests for the provide_time flag on APIClient / ConnectionParams."""As per coding guidelines: "Docstrings should be terse and default to single-line format ("""Summary.""") describing what the function does and what the caller can pass. Multi-line docstrings are the exception, only justified when there is non-obvious caller-visible behaviour that the type signature and parameter names don't already convey."
🤖 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_provide_time.py` around lines 1 - 5, Replace the multi-line module docstring at the top of the tests/test_provide_time.py file with a single-line terse docstring summarizing the module purpose (e.g., "Tests for the provide_time flag on APIClient/ConnectionParams."); update the existing triple-quoted block so it is one line only and preserves the same summary wording to follow project docstring conventions.
🤖 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.
Nitpick comments:
In `@tests/test_provide_time.py`:
- Around line 1-5: Replace the multi-line module docstring at the top of the
tests/test_provide_time.py file with a single-line terse docstring summarizing
the module purpose (e.g., "Tests for the provide_time flag on
APIClient/ConnectionParams."); update the existing triple-quoted block so it is
one line only and preserves the same summary wording to follow project docstring
conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c80c04e6-b939-49ed-97cd-b73e8c94862a
📒 Files selected for processing (1)
tests/test_provide_time.py
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…omeapi into provide-time
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_provide_time.py (2)
58-58: ⚡ Quick winMake the connection-patch helper private and keep helpers at the bottom.
patch_create_connectionis test-internal but publicly named. Rename it to_patch_create_connection; also keep private helpers grouped below public tests for consistency.🧹 Suggested rename
- with patch_create_connection(loop, transport, connected): + with _patch_create_connection(loop, transport, connected): connect_task = asyncio.create_task(connect(conn, login=False)) await connected.wait() send_plaintext_hello(conn._frame_helper) await connect_task return conn, transport, conn._frame_helper -def patch_create_connection(loop, transport, connected): +def _patch_create_connection(loop, transport, connected): return __import__("unittest.mock", fromlist=["patch"]).patch.object( loop, "create_connection", side_effect=partial(_create_mock_transport_protocol, transport, connected), )As per coding guidelines,
**/*.py: "Organize methods with public API at the top and private helpers (_underscore_prefixed) at the bottom."Also applies to: 66-71
🤖 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_provide_time.py` at line 58, The helper function patch_create_connection is test-internal but named and positioned like a public API; rename the function to _patch_create_connection and update all its call sites (e.g., the with patch_create_connection(loop, transport, connected): usages) to use the new name, and move the helper definition to the bottom of the file beneath the public tests so private helpers are grouped together per project guidelines.
1-5: ⚡ Quick winUse a terse single-line module docstring.
This docstring is multi-line and more verbose than needed for this test module.
✂️ Suggested simplification
-"""Tests for the provide_time flag on APIClient / ConnectionParams. - -When provide_time=True (the default) the connection registers a handler -for GetTimeRequest and responds to it, otherwise ignores the request. -""" +"""Tests provide_time behavior for GetTimeRequest handling."""As per coding guidelines,
**/*.py: "Docstrings should be terse and default to single-line format ("""Summary.""")."🤖 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_provide_time.py` around lines 1 - 5, Replace the multi-line module docstring at the top of tests/test_provide_time.py with a terse single-line docstring summarizing the test (e.g., a one-line description of the provide_time flag behavior); locate the module docstring that currently mentions provide_time, APIClient / ConnectionParams and GetTimeRequest and condense it to a single-line summary per project docstring guidelines.
🤖 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 `@tests/test_provide_time.py`:
- Around line 46-50: The return type annotation of _make_connected_conn is
wrong: it declares tuple[APIConnection, asyncio.Transport,
APIPlaintextFrameHelper, asyncio.Task] but the function actually returns (conn,
transport, conn._frame_helper); update the annotation on _make_connected_conn to
tuple[APIConnection, asyncio.Transport, APIPlaintextFrameHelper] (or
alternatively adjust the function to also return the asyncio.Task if intended),
ensuring the types match the actual returned variables conn, transport, and
conn._frame_helper.
---
Nitpick comments:
In `@tests/test_provide_time.py`:
- Line 58: The helper function patch_create_connection is test-internal but
named and positioned like a public API; rename the function to
_patch_create_connection and update all its call sites (e.g., the with
patch_create_connection(loop, transport, connected): usages) to use the new
name, and move the helper definition to the bottom of the file beneath the
public tests so private helpers are grouped together per project guidelines.
- Around line 1-5: Replace the multi-line module docstring at the top of
tests/test_provide_time.py with a terse single-line docstring summarizing the
test (e.g., a one-line description of the provide_time flag behavior); locate
the module docstring that currently mentions provide_time, APIClient /
ConnectionParams and GetTimeRequest and condense it to a single-line summary per
project docstring guidelines.
🪄 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: 64ed59dd-9f02-4fd0-875d-9a12a92d8b4f
📒 Files selected for processing (1)
tests/test_provide_time.py
|
@bluetoothbot review |
bdraco
left a comment
bdraco
left a comment
There was a problem hiding this comment.
LGTM assuming the bot doesn't find something I missed
I reconstructed this exact diff on top of current |
|
|
||
|
|
||
| def patch_create_connection(loop, transport, connected): | ||
| return __import__("unittest.mock", fromlist=["patch"]).patch.object( |
There was a problem hiding this comment.
This could be a bit cleaner and is inconsistant with the rest of the tests. Bot caught it above and had a suggestion.
What does this implement/fix?
Related to:
home-assistant/core#171846
esphome/esphome#14658
esphome/esphome#16583
Adds a flag to APIClient that will cause the connection to ignore time sync requests. The default value maintains the existing behaviour so is non-breaking.
Types of changes
Related issue or feature (if applicable):
Pull request in esphome (if applicable):
Checklist:
tests/folder).