Improvements #48
Improvements #48
Conversation
WalkthroughThis update introduces new fixture files, enhances sensitive data redaction in exception handling, and adds optional fields to data models. The device discovery function is renamed and its parameters are updated. Contribution and changelog documentation are revised to reflect new devices and features. Tests and fixture generation scripts are also updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AirOSDevice
participant Logger
participant DataRedactor
User->>AirOSDevice: status()
AirOSDevice->>AirOSDevice: Call status API
AirOSDevice->>AirOSDevice: Deserialize JSON
alt Deserialization fails (InvalidFieldValue or MissingField)
AirOSDevice->>DataRedactor: redact_data_smart(response_json)
DataRedactor-->>AirOSDevice: redacted_json
AirOSDevice->>Logger: Log error/exception with redacted_json
AirOSDevice->>User: Raise AirOSKeyDataMissingError
else Success
AirOSDevice->>User: Return parsed data
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
==========================================
+ Coverage 95.37% 95.70% +0.32%
==========================================
Files 9 9
Lines 1168 1281 +113
==========================================
+ Hits 1114 1226 +112
- Misses 54 55 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
script/generate_ha_fixture.py (1)
38-65: Refactor duplicated fixture-generation blocks & avoid pseudo-static call
Five nearly identicalwith openblocks violate DRY and rely on the hackyAirOS.derived_data(None, ...)call (instance method invoked withNone). Encapsulate the logic in a helper and iterate over source/target pairs; also create a realAirOS()instance (or makederived_dataa@staticmethod).-# Repeated blocks ... +fixtures = { + "airos_loco5ac_ap-ptp.json": "loco5ac_ap-ptp.json", + "airos_loco5ac_sta-ptp.json": "loco5ac_sta-ptp.json", + "airos_mocked_sta-ptmp.json": "mocked_sta-ptmp.json", + "airos_liteapgps_ap_ptmp_40mhz.json": "liteapgps_ap_ptmp_40mhz.json", + "airos_nanobeam5ac_sta_ptmp_40mhz.json": "nanobeam5ac_sta_ptmp_40mhz.json", +} + +air_os = AirOS() # proper instance + +for target, source in fixtures.items(): + src_path = os.path.join(userdata_dir, source) + dst_path = os.path.join(fixture_dir, target) + with open(src_path) as src_file, open(dst_path, "w") as dst_file: + data = json.load(src_file) + derived = air_os.derived_data(data) + json.dump(AirOSData.from_dict(derived).to_dict(), dst_file, indent=2, sort_keys=True)
🧹 Nitpick comments (7)
fixtures/userdata/nanobeam5ac_sta_ptmp_40mhz.json (1)
1-1: Consider formatting the JSON for better maintainability.While functionally correct, the single-line JSON format makes this fixture difficult to read and maintain. Consider formatting it with proper indentation for better developer experience.
-{"chain_names":[{"number":1,"name": "Chain 0"},{"number":2,"name": "Chain 1"}],"host":{"hostname": "NanoBeam-shed1",... +{ + "chain_names": [ + {"number": 1, "name": "Chain 0"}, + {"number": 2, "name": "Chain 1"} + ], + "host": { + "hostname": "NanoBeam-shed1", + ... + } +}fixtures/userdata/liteapgps_ap_ptmp_40mhz.json (1)
1-1: Consider formatting the JSON for better maintainability.Like the previous fixture, this single-line JSON format makes the file difficult to read and maintain. Proper indentation would improve developer experience when working with this test data.
fixtures/userdata/mocked_sta-ptmp.json (1)
1-1: Consider a standardized approach to fixture formatting.This is the third fixture file with single-line JSON format. Consider implementing a standardized approach to format all fixture files for better maintainability across the project.
You might want to create a script to format all fixture JSON files consistently:
# Format all fixture JSON files find fixtures/ -name "*.json" -exec python -m json.tool {} {}.tmp \; -exec mv {}.tmp {} \;CONTRIBUTE.md (1)
10-11: Ensure consistent device naming & ordering in fixture list
The two newly added bullets look good, but the list is no longer alphabetically sorted and the capitalisation differs from earlier entries (NanobeamvsNanoStation). Consider normalising the vendor-style casing (“NanoBeam”, “LiteAP GPS”) and, optionally, sorting the list to keep the section tidy.tests/test_discovery.py (1)
5-5: Drop the inline “Add this import” comment
Inline comments on import lines add noise once the change is merged. Remove the trailing comment to keep imports clean.-import socket # Add this import +import socketfixtures/airos_mocked_sta-ptmp.json (1)
29-30: Consider using generic test coordinates instead of real-world locations.The fixture contains what appear to be real GPS coordinates (Amsterdam area). For test fixtures, it's better to use clearly fake coordinates to avoid any confusion or potential privacy concerns.
- "lat": 52.379894, - "lon": 4.901608 + "lat": 0.0, + "lon": 0.0Also apply similar changes to lines 527-528, 538-539, 957-958, and 968-969.
Also applies to: 527-528, 538-539, 957-958, 968-969
fixtures/airos_liteapgps_ap_ptmp_40mhz.json (1)
29-30: Use consistent generic test coordinates across all fixtures.Similar to the previous fixture, this file contains what appear to be real GPS coordinates. For consistency and privacy, use generic test coordinates.
- "lat": 52.379894, - "lon": 4.901608 + "lat": 0.0, + "lon": 0.0Also apply to the remote GPS data at lines 538-539, 968-969, and 981-982.
Also applies to: 538-539, 968-969, 981-982
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
CHANGELOG.md(1 hunks)CONTRIBUTE.md(1 hunks)airos/airos8.py(2 hunks)airos/data.py(6 hunks)airos/discovery.py(2 hunks)fixtures/airos_liteapgps_ap_ptmp_40mhz.json(1 hunks)fixtures/airos_loco5ac_ap-ptp.json(2 hunks)fixtures/airos_loco5ac_sta-ptp.json(2 hunks)fixtures/airos_mocked_sta-ptmp.json(1 hunks)fixtures/airos_nanobeam5ac_sta_ptmp_40mhz.json(1 hunks)fixtures/userdata/liteapgps_ap_ptmp_40mhz.json(1 hunks)fixtures/userdata/mocked_invalid_wireless_mode.json(1 hunks)fixtures/userdata/mocked_missing_wireless_mode.json(1 hunks)fixtures/userdata/mocked_sta-ptmp.json(1 hunks)fixtures/userdata/nanobeam5ac_sta_ptmp_40mhz.json(1 hunks)pyproject.toml(1 hunks)script/generate_ha_fixture.py(1 hunks)tests/test_discovery.py(6 hunks)tests/test_stations.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/test_discovery.py (1)
airos/discovery.py (1)
airos_discover_devices(279-327)
airos/airos8.py (2)
airos/data.py (1)
redact_data_smart(62-109)airos/exceptions.py (3)
AirOSKeyDataMissingError(20-21)AirOSDataMissingError(16-17)AirOSDeviceConnectionError(24-25)
fixtures/airos_mocked_sta-ptmp.json (1)
tests/conftest.py (1)
airos_device(20-25)
script/generate_ha_fixture.py (1)
airos/airos8.py (2)
derived_data(195-248)AirOS(25-386)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Process test coverage
🔇 Additional comments (22)
airos/discovery.py (2)
279-281: LGTM! Good API enhancement.The function rename from
async_discover_devicestoairos_discover_devicesfollows better naming conventions, and adding configurablelisten_ipandportparameters with sensible defaults enhances flexibility while maintaining backward compatibility.
306-306: Correct implementation of new parameters.The new parameters are properly used in the
create_datagram_endpointcall, replacing the hardcoded values with the configurable ones.pyproject.toml (1)
7-7: LGTM! Appropriate version bump.The version update to
0.2.6a1correctly reflects the pre-release alpha status and aligns with the new features and enhancements introduced in this pull request.fixtures/airos_loco5ac_ap-ptp.json (1)
171-177:flex_modeadded as null – verify dataclass acceptsNone
Ifflex_modehas been added to thePollingdataclass, confirm it is declared asOptional[bool | int](or similar) so that anullvalue won’t raise a validation error duringfrom_dict().CHANGELOG.md (1)
5-17: Changelog entry LGTM
Entry is clear and links to the contributing guide. No issues spotted.tests/test_discovery.py (1)
164-173: Good fix – explicitsocketimport resolves type-spec issue
Importingsocketeliminates the previous NameError when accessingsocket.SOL_SOCKETin the transport test.fixtures/airos_loco5ac_sta-ptp.json (2)
171-171: LGTM: Added flex_mode field to polling data.The addition of
"flex_mode": nullaligns with the enhancedPollingdataclass mentioned in the AI summary, providing consistent structure for optional polling parameters.
523-530: LGTM: Enhanced GPS data structure with additional optional fields.The new GPS fields (
alt,dim,dop,sats,time_synced) provide comprehensive GPS metadata including altitude, dimension, dilution of precision, satellite count, and synchronization status. Setting these tonullis appropriate for optional fields in test fixtures.fixtures/userdata/mocked_missing_wireless_mode.json (1)
35-67: LGTM: Well-structured fixture for testing missing wireless mode scenarios.This fixture correctly omits the wireless
modefield to simulate missing wireless mode data, which is valuable for testing error handling and data validation. The comprehensive device data provides realistic context for these test scenarios.fixtures/airos_nanobeam5ac_sta_ptmp_40mhz.json (1)
1-652: LGTM: Comprehensive fixture for NanoBeam 5AC device testing.This fixture provides detailed device data for a NanoBeam 5AC in station point-to-multipoint mode with 40MHz bandwidth. The structure follows established AIROS fixture patterns and includes comprehensive metrics including AirMax data, GPS information, and detailed station statistics, making it valuable for device-specific testing scenarios.
airos/airos8.py (3)
13-13: LGTM: Added redaction utility import.The import of
redact_data_smartenables secure logging of sensitive data, which is used effectively in the enhanced exception handling below.
271-296: Excellent enhancement to exception handling and data privacy.The restructured exception handling provides several improvements:
- Separate handling for
InvalidFieldValuevsMissingFieldexceptions with appropriate logging levels- Data redaction using
redact_data_smartprotects sensitive information like MAC addresses and IPs in logs- Cleaner control flow with the return statement outside the try-except block
- Consistent error mapping to
AirOSKeyDataMissingErrorfor both deserialization failuresThis significantly improves both error handling robustness and data privacy compliance.
298-303: LGTM: Improved logging levels for error conditions.Changing from
_LOGGER.exceptionto_LOGGER.errorfor HTTP errors and client connection errors is appropriate - these are expected error conditions that don't require full stack traces, reducing log noise while maintaining necessary error information.Also applies to: 308-308
fixtures/userdata/mocked_invalid_wireless_mode.json (2)
37-37: LGTM: Intentionally invalid wireless mode for testing validation.The
"sta-ptp-INVALID"value correctly provides an invalid wireless mode for testing data validation and error handling scenarios. This complements the missing wireless mode fixture and enhances test coverage for edge cases.
1-159: LGTM: Comprehensive fixture for invalid wireless mode testing.This fixture provides realistic device data with an intentionally invalid wireless mode value, enabling thorough testing of validation logic and error handling pathways. The structure is consistent with other userdata fixtures while serving the specific purpose of testing invalid data scenarios.
fixtures/airos_mocked_sta-ptmp.json (1)
533-533: Good test coverage for optional mode field.The fixture effectively tests the handling of the optional
modefield in remote devices - one withmode: null(line 540) and one withmode: "ap-ptmp"(line 970). The hostnames clearly indicate the test scenario which aids in understanding the test purpose.Also applies to: 540-540, 963-963, 970-970
tests/test_stations.py (2)
31-83: Well-structured test coverage for exception handling.The new tests effectively cover both the redaction functionality and error logging paths. The mock setup is comprehensive and the assertions properly verify the expected behavior.
Also applies to: 85-123
47-47: Incorrect missing fixture warningThe fixture file
fixtures/userdata/mocked_invalid_wireless_mode.jsonis present, so the warning about a missing"mocked_invalid_wireless_mode"fixture can be ignored.Likely an incorrect or invalid review comment.
fixtures/airos_liteapgps_ap_ptmp_40mhz.json (1)
38-38: Good coverage of new optional fields.The fixture properly tests the new optional fields:
height: nullat line 38 (Host dataclass)flex_mode: 1at line 275 (Polling dataclass)- Extended GPS fields in remote sections with null values for LiteAP-specific fields
This provides good test coverage for the data model changes.
Also applies to: 275-275, 334-338, 534-542, 964-973
airos/data.py (3)
331-338: Well-structured optional field additions.The changes to make GPS fields, mode, height, and other fields optional are well thought out:
- Proper use of
| Nonetype hints with default values- Comments explaining why fields are optional (e.g., "Investigate why remotes can have no mode set")
- Aligns with the fixture data showing these fields can be null
Also applies to: 389-391, 490-490
396-396: Good fix for the incorrect log message.The correction from "Wireless" to "Remote" in the log message ensures accurate debugging information.
62-110: Robust implementation of smart data redaction.The
redact_data_smartfunction is well-implemented with:
- Comprehensive list of sensitive keys
- Proper handling of nested dictionaries and lists
- Smart detection of MAC addresses, IP addresses, and other sensitive data
- Maintains data structure while redacting values
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores