Tag/improve quality #71
Conversation
|
Warning Rate limit exceeded@CoMPaTech has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughDocumentation, tests, and tooling configs updated; minor cosmetic and logging changes to core modules. Discovery parser raises now suppress exception chaining. New mypy/ruff/pytest/coverage configurations and test dependency added. Scripts receive lint suppressions; tests reorder imports and tighten some assertions. No public API signature changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as caller (status / _request_json)
participant AIROS as airos8._request_json
participant HTTP as aiohttp
participant Parser as AirOSData.from_dict
Caller->>AIROS: call status()
AIROS->>HTTP: perform HTTP request
alt request raises TimeoutError / ClientError
HTTP-->>AIROS: exception
AIROS->>Caller: log "Error during API call to <URL>" and raise AirOSDeviceConnectionError
else request returns JSON
AIROS->>Parser: AirOSData.from_dict(json)
Parser-->>Caller: AirOSData
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (29)
requirements-test.txt (1)
14-14: Pydantic v2 compliance confirmed
Ran a code-wide scan for Pydantic v1–era imports and APIs; no instances ofpydantic.v1,BaseModel,BaseSettings,validator,root_validator, etc., were found.• Optional: dedupe duplicate entries for
aiofilesandaioresponsesin requirements-test.txt to avoid confusion.CHANGELOG.md (1)
9-9: Tighten the phrasing for clarity.The current bullet reads a bit awkwardly. Suggest a minor wording tweak.
-- Aligned quality targets either improved or tagged +- Aligned quality targets: improved or tagged.script/generate_discovery_fixture.py (2)
11-13: Prefer pathlib over os.path; drop the noqa suppressions.Using pathlib simplifies path handling and removes the need for PTH nox-qa.
Apply this diff to the selected lines:
-fixture_dir = os.path.join(os.path.dirname(__file__), "../fixtures") # noqa: PTH118, PTH120 -os.makedirs(fixture_dir, exist_ok=True) # Ensure the directory exists # noqa: PTH103 -fixture_path = os.path.join(fixture_dir, "airos_sta_discovery_packet.bin") # noqa: PTH118 +fixture_dir = Path(__file__).resolve().parent.parent / "fixtures" +fixture_dir.mkdir(parents=True, exist_ok=True) # Ensure the directory exists +fixture_path = fixture_dir / "airos_sta_discovery_packet.bin"And update imports (outside this hunk) to add pathlib and optionally drop os:
from pathlib import Path # import os # can be removed if unused elsewhere
97-97: Use Path.open to remove the PTH123 suppression.This keeps behavior identical and removes the noqa.
-with open(fixture_path, "wb") as f: # noqa: PTH123 +with fixture_path.open("wb") as f:airos/data.py (1)
33-40: Harden is_ip_address against non-str inputs and stray whitespace.Callers may pass non-strs; ipaddress.ip_address(None) raises TypeError. Trimming whitespace avoids false negatives.
-def is_ip_address(value: str) -> bool: +def is_ip_address(value: str) -> bool: """Check if a string is a valid IPv4 or IPv6 address.""" try: - ipaddress.ip_address(value) - except ValueError: + ipaddress.ip_address(value.strip() if isinstance(value, str) else value) + except (ValueError, TypeError): return False return Truescript/mashumaro-step-debug.py (5)
26-26: Prefer SystemExit (or argparse) over raising bare ExceptionRaising a broad Exception for CLI argument errors is noisy and flagged by linters. Use SystemExit or argparse to provide a clean exit code/message.
- raise Exception("File to check not provided.") # noqa: TRY002 + raise SystemExit("File to check not provided.")
28-33: Remove redundant sys.path setup inside main()current_script_dir/project_root_dir and sys.path mutation are already done at module import (Lines 9-13). This duplication is unnecessary.
- current_script_dir = os.path.dirname(os.path.abspath(__file__)) # noqa: PTH100, PTH120 - project_root_dir = os.path.abspath(os.path.join(current_script_dir, os.pardir)) # noqa: PTH100, PTH118 - - if project_root_dir not in sys.path: - sys.path.append(project_root_dir) + # Project path already added at module import; no need to repeat here.
35-36: Use json.load to read file directlySlight simplification and marginally better memory behavior.
- with open(sys.argv[1], encoding="utf-8") as f: # noqa: PTH123 - data = json.loads(f.read()) + with open(sys.argv[1], encoding="utf-8") as f: # noqa: PTH123 + data = json.load(f)
49-61: Avoid building an unused liststation_obj_list is populated but never used. Iterate without accumulating to reduce noise.
- station_list_data = wireless_data["sta"] - station_obj_list = [] + station_list_data = wireless_data["sta"] for i, station_data in enumerate(station_list_data): _LOGGER.info(" -> Checking Station object at index %s...", i) remote_data = station_data["remote"] _LOGGER.info(" -> Checking Remote object at index %s...", i) _LOGGER.info("Remote data = %s", remote_data) remote_obj = Remote.from_dict(remote_data) # noqa: F841 _LOGGER.info(" Success! Remote is valid.") station_obj = Station.from_dict(station_data) - station_obj_list.append(station_obj) + # Deserialization succeeded; no need to store the object. _LOGGER.info(" Success! Station at index %s is valid.", i)
55-56: Check for sensitive data in logsRemote data may include identifiers or credentials; ensure this script isn’t used in contexts where logs are collected centrally.
script/generate_ha_fixture.py (4)
26-31: Normalize fixture pathsJoining with "../..." can leave parent segments; normalize with abspath for predictable locations.
- fixture_dir = os.path.join(os.path.dirname(__file__), "../fixtures") # noqa: PTH118, PTH120 - userdata_dir = os.path.join(os.path.dirname(__file__), "../fixtures/userdata") # noqa: PTH118, PTH120 + fixture_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), "../fixtures")) # noqa: PTH118, PTH120 + userdata_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), "../fixtures/userdata")) # noqa: PTH118, PTH120
33-33: Deterministic iteration orderSorting ensures stable fixture generation across runs.
- for filename in os.listdir(userdata_dir): # noqa: PTH208 + for filename in sorted(os.listdir(userdata_dir)): # noqa: PTH208
45-46: Use json.load for simplicityMinor cleanup; avoids reading the entire file into a string first.
- with open(base_fixture_path, encoding="utf-8") as source: # noqa: PTH123 - source_data = json.loads(source.read()) + with open(base_fixture_path, encoding="utf-8") as source: # noqa: PTH123 + source_data = json.load(source)
33-53: Consider skipping invalid files instead of aborting the whole runCurrently, a single invalid JSON or processing error raises and stops all further processing. For bulk fixture generation, it’s often more useful to log and continue.
Apply the diffs above; additionally, adjust the exception handling (outside this hunk) like so:
# Outside selected range: replace the two 'raise' statements to continue processing except json.JSONDecodeError: _LOGGER.error("Skipping '%s': Not a valid JSON file.", filename) continue except Exception as e: _LOGGER.error("Error processing '%s': %s", filename, e) continuepyproject.toml (2)
751-759: Per-file ignores for scripts and temporary PTH suppressionsReasonable tradeoff for DX; revisit the temporary PTH ignores when pathlib refactors are feasible.
374-551: PyTest warnings filter is extensive; ensure it doesn’t hide relevant deprecationsLarge ignore lists copied from other ecosystems (e.g., Home Assistant) can mask actionable warnings in this project. Consider trimming over time.
Would you like me to propose a minimal filter set based on current test output to avoid over-suppressing?
tests/conftest.py (1)
3-3: Use public typing module path (nitpick)Prefer collections.abc over the private _collections_abc to avoid relying on internal modules.
Apply this diff:
-from _collections_abc import AsyncGenerator, Generator +from collections.abc import AsyncGenerator, Generatorairos/airos8.py (2)
263-273: Likely content-type/payload mismatch in update_check(force=True)When force is True, ct_form=True is set but the payload is passed via json_data, not form_data. This can lead to inconsistent headers/body and server-side parsing issues.
Apply this diff:
- if force: - return await self._request_json( - "POST", - self._update_check_url, - json_data={"force": True}, - authenticated=True, - ct_form=True, - ) + if force: + return await self._request_json( + "POST", + self._update_check_url, + form_data={"force": True}, + authenticated=True, + ct_form=True, + )
122-123: Typo in comment (nitpick)“Fist” → “first”.
Apply this diff:
- # Fallback take fist alternate interface found + # Fallback: take first alternate interface foundairos/discovery.py (5)
32-33: Make the callback type hint asynchronous (Awaitable) to match create_task usage
asyncio.create_task(self.callback(...))expects an awaitable; declare the callback accordingly for type-safety and clearer intent.-from typing import Any +from typing import Any, Awaitable - def __init__(self, callback: Callable[[dict[str, Any]], Any]) -> None: + def __init__(self, callback: Callable[[dict[str, Any]], Awaitable[Any]]) -> None:
60-63: Log exceptions raised by the scheduled callback taskIf the callback coroutine raises after being scheduled, the exception will surface as an unhandled task exception. Attach a done-callback to log failures.
- # Schedule the user-provided callback, don't await to keep listener responsive - asyncio.create_task(self.callback(parsed_data)) # noqa: RUF006 + # Schedule the user-provided callback, don't await to keep listener responsive + task = asyncio.create_task(self.callback(parsed_data)) # noqa: RUF006 + task.add_done_callback( + lambda t: _LOGGER.exception("Discovery callback failed", exc_info=t.exception()) + if t.exception() # type: ignore[truthy-bool] + else None + )
269-274: Exception-chaining policy differs here vs. other malformed TLV raisesThis path still sets
__cause__(from err) while other TLV-parse raises suppress chaining (from None). If that’s intentional (to preserve low-level struct/index errors), consider adding a short comment. If not, align with the rest.- log = f"Malformed packet: {log}" - raise AirOSEndpointError(log) from err + log = f"Malformed packet: {log}" + # Preserve or suppress cause? Align with project policy. + raise AirOSEndpointError(log) from errIf you prefer to suppress chaining here too:
- raise AirOSEndpointError(log) from err + raise AirOSEndpointError(log) from None # noqa: TRY301
319-323: Use errno constants instead of magic numbers for portabilityComparing to raw
98is Linux-specific. Useerrno.EADDRINUSE.+import errno @@ - if err.errno == 98: + if err.errno == errno.EADDRINUSE: _LOGGER.error("Address in use, another instance may be running.") raise AirOSEndpointError("address_in_use") from err
88-104: Narrow the return type of parse_airos_packet; it never returns NoneThe implementation always returns a dict or raises. Tightening the type and simplifying the caller removes dead branches and clarifies semantics.
- def parse_airos_packet(self, data: bytes, host_ip: str) -> dict[str, Any] | None: + def parse_airos_packet(self, data: bytes, host_ip: str) -> dict[str, Any]: @@ - A dictionary containing parsed device information if successful, - otherwise None. Values will be None if not found or cannot be parsed. + A dictionary containing parsed device information. + Values will be None if not found or cannot be parsed.And in datagram_received:
- parsed_data: dict[str, Any] | None = self.parse_airos_packet(data, host_ip) - if parsed_data: - # Schedule the user-provided callback, don't await to keep listener responsive - asyncio.create_task(self.callback(parsed_data)) # noqa: RUF006 + parsed_data: dict[str, Any] = self.parse_airos_packet(data, host_ip) + # Schedule the user-provided callback, don't await to keep listener responsive + asyncio.create_task(self.callback(parsed_data)) # noqa: RUF006Also applies to: 60-63
tests/test_discovery.py (5)
114-117: Byte-literal concatenation style: prefer consistencyMixing implicit literal concatenation with explicit
+reads fine but is stylistically uneven. Consider using one approach throughout the block for readability.Example (explicit all the way):
- truncated_data = ( - b"\x01\x06\x00\x00\x00\x00" # Header - b"\x06" - + bytes.fromhex("0123456789CD") # Valid MAC (scrubbed) - + b"\x02\x00" # TLV type 0x02, followed by only 1 byte for length (should be 2) - ) + truncated_data = ( + b"\x01\x06\x00\x00\x00\x00" # Header + + b"\x06" + + bytes.fromhex("0123456789CD") # Valid MAC (scrubbed) + + b"\x02\x00" # TLV type 0x02, followed by only 1 byte for length (should be 2) + )
431-433: Nit: Keep bytes concatenation style consistent here as wellMirror the earlier suggestion for consistent explicit concatenation.
- data_with_fragment = ( - b"\x01\x06\x00\x00\x00\x00" - b"\x06" + data_with_fragment = ( + b"\x01\x06\x00\x00\x00\x00" + + b"\x06" + bytes.fromhex("0123456789CD") + b"\x0a\x00" # TLV type 0x0a, followed by only 1 byte for length (should be 2) )
452-454: Nit: Consistent bytes concatenationSame consistency suggestion as above.
- data_with_fragment = ( - b"\x01\x06\x00\x00\x00\x00" - b"\x06" + data_with_fragment = ( + b"\x01\x06\x00\x00\x00\x00" + + b"\x06" + bytes.fromhex("0123456789CD") + b"\x02\x00" # TLV type 0x02, followed by only 1 byte for length (should be 2) )
259-265: Optionally assert the function’s return value in the success pathYou already capture discovered devices via the patched protocol callback. To strengthen the contract, also assert the dict returned by
airos_discover_devices.- discovery_task = asyncio.create_task(airos_discover_devices(timeout=1)) + discovery_task = asyncio.create_task(airos_discover_devices(timeout=1)) @@ - await discovery_task + result = await discovery_task + # Validate return value mirrors discovered devices + assert "01:23:45:67:89:CD" in result + assert result["01:23:45:67:89:CD"]["hostname"] == "name"
436-442: Consider asserting suppressed exception chaining where applicableFor paths intentionally using
raise ... from None, assert__cause__ is Noneto lock in the behavior.- with pytest.raises(AirOSEndpointError): - protocol.parse_airos_packet(data_with_fragment, host_ip) + with pytest.raises(AirOSEndpointError) as excinfo: + protocol.parse_airos_packet(data_with_fragment, host_ip) + assert excinfo.value.__cause__ is None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
CHANGELOG.md(1 hunks)airos/airos8.py(2 hunks)airos/data.py(1 hunks)airos/discovery.py(6 hunks)mypy.ini(1 hunks)pyproject.toml(4 hunks)requirements-test.txt(1 hunks)script/__init__.py(1 hunks)script/generate_discovery_fixture.py(2 hunks)script/generate_ha_fixture.py(2 hunks)script/mashumaro-step-debug.py(3 hunks)tests/conftest.py(1 hunks)tests/test_airos8.py(1 hunks)tests/test_airos_request.py(6 hunks)tests/test_data.py(1 hunks)tests/test_discovery.py(9 hunks)tests/test_stations.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
airos/airos8.py (1)
airos/exceptions.py (1)
AirOSDeviceConnectionError(24-25)
tests/test_data.py (1)
airos/data.py (2)
Host(190-213)Wireless(448-496)
tests/test_airos8.py (1)
airos/airos8.py (1)
AirOS(32-349)
script/generate_ha_fixture.py (1)
airos/airos8.py (2)
derived_data(77-134)AirOS(32-349)
airos/discovery.py (1)
airos/exceptions.py (1)
AirOSEndpointError(36-37)
tests/test_airos_request.py (1)
airos/airos8.py (1)
_request_json(169-227)
tests/conftest.py (2)
airos/airos8.py (1)
AirOS(32-349)airos/discovery.py (1)
AirOSDiscoveryProtocol(18-280)
tests/test_discovery.py (2)
airos/exceptions.py (1)
AirOSEndpointError(36-37)airos/discovery.py (1)
airos_discover_devices(283-331)
🔇 Additional comments (22)
script/__init__.py (1)
1-1: LGTM on the package marker docstring.Clear, concise, and harmless.
mypy.ini (3)
1-26: Strict mypy configuration looks solidGood balance of strictness and practicality; warnings and error codes are tuned appropriately for a production codebase.
4-4: Validate Pydantic plugin selectionBoth pydantic.mypy and pydantic.v1.mypy are enabled. Depending on your installed Pydantic versions, loading both may be redundant. Confirm that CI has a plugin-compatible Pydantic and that mypy isn't emitting plugin load warnings.
Would you like a quick check added to CI to print mypy’s loaded plugins for the job log?
36-44: Test-specific relaxations are appropriateRelaxed rules under [mypy-tests.*] align with typical test code patterns and should reduce false positives.
pyproject.toml (1)
57-59: Enabling pylint code_style and typing extensions is a good moveThese plugins catch useful issues; nice addition.
tests/test_data.py (1)
7-8: Import reordering LGTMMatches the repository’s linting preferences; no behavioral changes.
tests/test_stations.py (1)
9-11: Import consolidation at top-level looks goodMoving aiofiles, MissingField, and pytest to the top eliminates duplicates and aligns with the broader test-suite organization/linting changes.
tests/conftest.py (2)
7-7: Add aiohttp import at top-level — necessary and correctThis prevents NameError in the aiohttp-based fixture and matches the updated import policy across tests.
10-12: Reordered imports — consistent with pytest-first orderingImporting AirOS and AirOSDiscoveryProtocol after pytest is fine and helps ensure decorators/fixtures are available.
tests/test_airos8.py (2)
10-14: Import reordering and spacing — OKBringing pytest earlier and placing AirOS/imports after it is consistent; no runtime changes.
281-283: Cause chaining assertion matches current implementationairos/airos8.py raises AirOSKeyDataMissingError from MissingField; asserting cause is MissingField is correct and valuable for diagnostics.
airos/airos8.py (2)
220-221: Shorter connection-error log with full traceback — good tradeoffSwitching to _LOGGER.exception("Error during API call to %s", url) keeps the traceback while shortening the message content. Behavior of raising AirOSDeviceConnectionError from err remains intact.
243-246: Direct return from AirOSData.from_dict — OKStreamlines without changing behavior; exceptions are still handled below.
tests/test_airos_request.py (2)
6-8: Top-level aiohttp/pytest imports — consistent and fineMatches the repo-wide test import reorg; no behavior change.
51-51: Accessing private method in tests with SLF001 suppression is acceptableUsing _request_json directly in tests is reasonable; the selective # noqa: SLF001 keeps lints clean without weakening production code.
Also applies to: 78-79, 102-103, 125-126, 147-149
airos/discovery.py (1)
154-155: LGTM: Suppressing exception chaining on malformed TLV raises is consistentUsing
raise ... from None # noqa: TRY301for TLV parse errors keeps logs concise and matches the stated intent. Please double-check whether you want the same policy for all parse-error paths (see struct/index catch below).Also applies to: 168-173, 184-185, 195-200, 216-221, 267-268
tests/test_discovery.py (6)
10-11: LGTM: pytest import addedNeeded for fixture utilities and assertions; matches usage below.
37-38: LGTM: Narrowing exception to OSError is appropriateCatching only file I/O errors clarifies intent and avoids swallowing unrelated exceptions.
293-301: LGTM: Robust simulation of EADDRINUSE during endpoint creationPatching
get_running_loop().create_datagram_endpointto raiseOSError(98, ...)and assertingAirOSEndpointError("address_in_use")matches the production flow.
354-355: LGTM: Parametrization uses tuple of arg namesSwitching to tuple form improves readability and avoids comma-in-string pitfalls.
388-396: LGTM: Generic OSError path coverageGood separation from the EADDRINUSE case; asserting
AirOSEndpointError("cannot_connect")aligns with the code.
466-471: LGTM: Parametrization tuple for unhandled TLVsClearer argument naming; matches test body expectations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pyproject.toml (1)
552-554: Coverage source fixed to target airos (good catch)This addresses the earlier review correctly. Expect coverage to reflect project modules now.
🧹 Nitpick comments (4)
pyproject.toml (4)
343-348: Pylint per-file-ignores path matching may not trigger with a leading slashPylint’s per-file-ignores typically matches relative paths/globs (e.g., tests/*). The leading slash in "/tests/:" may prevent matches depending on runner CWD. Consider removing it.
Apply this diff:
-per-file-ignores = [ - # redefined-outer-name: Tests reference fixtures in the test function - # use-implicit-booleaness-not-comparison: Tests need to validate that a list - # or a dict is returned - "/tests/:redefined-outer-name,use-implicit-booleaness-not-comparison", -] +per-file-ignores = [ + # redefined-outer-name: Tests reference fixtures in the test function + # use-implicit-booleaness-not-comparison: Tests need to validate that a list + # or a dict is returned + "tests/**:redefined-outer-name,use-implicit-booleaness-not-comparison", +]
374-551: Pytest warnings filter block is very broad; verify necessity per project scopeThere are many HA- and third-party-specific suppressions. If they’re not exercised by airos tests, trimming reduces maintenance burden and risk of hiding real issues.
Would you like me to propose a minimal set derived from current test runs?
555-567: Consider enabling branch coverage and excluding typical “if name == 'main':” guardsEnabling branch coverage yields more accurate metrics; excluding main guards avoids noise.
Apply this diff:
[tool.coverage.run] -source = ["airos"] +source = ["airos"] +branch = true +relative_files = true + [tool.coverage.report] exclude_lines = [ # Have to re-enable the standard pragma "pragma: no cover", + # Don't complain about script entry points: + "if __name__ == .__main__.:", # Don't complain about missing debug-only code:
590-594: Ruff “C” selector is ambiguous; prefer explicit familiesIf you intend flake8-comprehensions use “C4”; McCabe is “C90” (already configured via mccabe). Using bare “C” can be unclear across Ruff versions.
Apply this diff:
- "BLE", - "C", # complexity + "BLE", + "C4", # flake8-comprehensionsIf you also want McCabe via select (not only config), add:
+ "C90", # mccabe (complexity)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pyproject.toml(4 hunks)
🔇 Additional comments (9)
pyproject.toml (9)
7-7: Confirm pre-release tagging (0.4.2a0) is intentionalAlpha tag will publish as a pre-release on PyPI. Double-check release process (changelog, CI publish conditions) to avoid unintentionally pushing a pre-release.
57-59: Pylint plugins addition looks goodEnabling code_style and typing extensions is appropriate given the Ruff/Pylint split that follows.
355-356: attrs typing quirk handledIgnoring _CountingAttr for attrs is a common/harmless tweak. LGTM.
736-739: pytest-style mark-parentheses turned off intentionallyThis aligns with common pytest ergonomics; no concerns.
741-744: Banned API rules are sensible
- async_timeout -> asyncio.timeout
- Prevent importing tests
Solid guardrails.
746-748: isort known-first-party set to “airos”Correctly informs import sorter. Good.
751-759: Temporary PTH ignores for airos/testsReasonable while migrating to pathlib. Revisit later to re-enable.
Consider opening a tracking issue to remove these ignores once migration is complete.
761-762: Complexity threshold configuredmax-complexity = 25 is pragmatic for this codebase. No issues.
764-765: Docstring convention switched to Google with property decorator allowanceConsistent with modern style guides; works well with cached_property configuration.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #71 +/- ##
=======================================
Coverage ? 91.36%
=======================================
Files ? 4
Lines ? 730
Branches ? 0
=======================================
Hits ? 667
Misses ? 63
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.pre-commit-config.yaml (1)
95-95: Pytest logging flag is fine; optional: let pytest.ini drive logging to avoid duplicationAdding --log-level info is valid and will surface useful logs locally. If you already define logging in pyproject/pytest.ini, consider relying on that to keep a single source of truth.
Optional diff to delegate logging config to pytest.ini:
- entry: script/run-in-env.sh pytest --log-level info + entry: script/run-in-env.sh pytest.github/workflows/verify.yml (1)
99-99: Narrowing coverage to the package is good; optional: prefer package name over path and rely on pytest testpathsTargeting airos/ avoids counting tests in coverage—nice. For portability and to respect pytest’s configured test discovery (testpaths), consider:
- using --cov=airos (package name) instead of a path, and
- omitting tests/*.py so pytest uses testpaths from config (helps if tests are reorganized later).
Suggested update:
- pytest --log-level info tests/*.py --cov='airos/' + pytest --log-level info --cov=airos
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/verify.yml(1 hunks).pre-commit-config.yaml(1 hunks)
⏰ 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: Run pytest using Python 3.13
|
1 participant
Summary by CodeRabbit
Changed
Documentation
Tests
Style
Chores