Skip to content

Conversation

@balloob
Copy link
Member

@balloob balloob commented Sep 19, 2025

What does this implement/fix?

Make the port and noise PSK available.

Types of changes

  • Bugfix (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 improvements to existing code or addition of tests
  • Other

Related issue or feature (if applicable):

  • fixes

Pull request in esphome (if applicable):

  • esphome/esphome#

Checklist:

  • The code change is tested and works locally.
  • If api.proto was modified, a linked pull request has been made to esphome with the same changes.
  • Tests have been added to verify that the new code works (under tests/ folder).

@codecov
Copy link

codecov bot commented Sep 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (c1140a4) to head (cfcfb3d).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1352   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         3222      3228    +6     
=========================================
+ Hits          3222      3228    +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bdraco
Copy link
Member

bdraco commented Sep 19, 2025

Could use coverage but if you need it now I can always add it later

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 19, 2025

CodSpeed Performance Report

Merging #1352 will not alter performance

Comparing client-port-noise (cfcfb3d) with main (c1140a4)

Summary

✅ 11 untouched

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 19, 2025

Walkthrough

Adds two read-only properties to APIClientBase exposing connection parameters: port (int) and noise_psk (str | None). Also updates a test to assert the new properties. No setters, no behavioral or control-flow changes.

Changes

Cohort / File(s) Change Summary
Client public API additions
aioesphomeapi/client_base.py
Added read-only properties: port returning self._params.port and noise_psk returning self._params.noise_psk.
Tests updated
tests/test_client.py
Renamed/expanded test to test_client_properties and added assertions for expected_name, address, port, noise_psk, and api_version, while still checking connected_address.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

new-feature, minor

Suggested reviewers

  • bdraco

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title accurately and concisely describes the primary change by stating that port and noise_psk properties are being added to client_base, which matches the modifications in aioesphomeapi/client_base.py. It is specific to the main change, brief, and free of noise or irrelevant details. A reviewer scanning PR history will understand the primary intent from this title.
Description Check ✅ Passed The PR description states the intent to make the port and noise PSK available and marks the change as a non-breaking new feature, which aligns with the code changes and updated tests. Though brief, the description is on-topic and references relevant checklist items and considerations for api.proto and esphome linkage. It therefore satisfies the lenient requirement of being related to the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch client-port-noise

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 and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
aioesphomeapi/client_base.py (2)

299-302: Expose port: LGTM. Add a short docstring for clarity.

Clear and consistent with existing address/connected_address accessors. Consider a one‑liner docstring to pin semantics (configured port vs. currently connected port).

 @property
 def port(self) -> int:
-    return self._params.port
+    """Configured TCP port used to establish the API connection."""
+    return self._params.port

303-306: Mark noise_psk as secret and avoid logging.

Add a docstring to flag this property as secret; quick repo scan found no logger calls referencing noise_psk.

 @property
 def noise_psk(self) -> str | None:
-    return self._params.noise_psk
+    """Noise PSK (secret). Do not log or expose in user-visible output."""
+    return self._params.noise_psk

Location: aioesphomeapi/client_base.py:303-306. Other occurrences: aioesphomeapi/connection.py, aioesphomeapi/_frame_helper/noise.py, and tests/ (sample PSKs).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c1140a4 and a29659b.

📒 Files selected for processing (1)
  • aioesphomeapi/client_base.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

  • Do not generate or add any sequence diagrams

Files:

  • aioesphomeapi/client_base.py

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
tests/test_client.py (2)

1210-1210: Docstring no longer matches the test scope

Update to reflect all properties being verified.

-    """Test getting the connected address."""
+    """Test client properties: connected_address, expected_name, address, port, noise_psk, api_version."""

1215-1216: Optional: add a quick type assertion for port

Minor hardening to ensure the property exposes an int.

     assert client.port == 6052
+    assert isinstance(client.port, int)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a29659b and cfcfb3d.

📒 Files selected for processing (1)
  • tests/test_client.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

  • Do not generate or add any sequence diagrams

Files:

  • tests/test_client.py
🧬 Code graph analysis (1)
tests/test_client.py (3)
tests/conftest.py (1)
  • api_client (209-238)
aioesphomeapi/client_base.py (7)
  • connected_address (308-312)
  • expected_name (288-289)
  • expected_name (292-293)
  • address (296-297)
  • port (300-301)
  • noise_psk (304-305)
  • api_version (315-318)
aioesphomeapi/model.py (1)
  • APIVersion (100-102)
🔇 Additional comments (1)
tests/test_client.py (1)

1205-1217: LGTM: new client properties are exercised correctly

Assertions cover connected_address, expected_name, address, port, noise_psk, and api_version as intended.

@bdraco bdraco merged commit 97d59a9 into main Sep 19, 2025
15 checks passed
@bdraco bdraco deleted the client-port-noise branch September 19, 2025 01:26
@balloob balloob added the minor label Sep 19, 2025
@bdraco
Copy link
Member

bdraco commented Sep 19, 2025

Good timing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants