Add pydoclint documentation checks - #95
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe pull request standardizes Google-style docstrings across production code, scripts, and tests. It adds explicit type documentation and selected test annotations. It also adds pydoclint tooling and project configuration. Runtime behavior remains unchanged. ChangesDocumentation and linting
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
github-actions
Bot
added
documentation
Contributor
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Documentation build overview
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_vpn.py (2)
85-93: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winType and rename the unused list-getter parameter.
fake_safe_list_getdoes not readpath, so Ruff reportsARG001. Rename it to_path, add-> list[object], and update theArgssection.🤖 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_vpn.py` around lines 85 - 93, Update the fake_safe_list_get test helper by renaming its unused path parameter to _path, adding the return annotation list[object], and revising the Args documentation to use the new parameter name.Source: Linters/SAST tools
59-67: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd return annotations to the nested dictionary fakes.
fake_safe_dict_gethas no return annotation at Lines [59], [226], [268], and [447]. Add-> dict[str, Any]to each helper. Ruff reportsANN202.As per coding guidelines, every Python function must include a return annotation.
Also applies to: 226-234, 268-276, 447-455
🤖 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_vpn.py` around lines 59 - 67, Update each nested fake_safe_dict_get helper at the four occurrences in tests/test_vpn.py to declare the return type as dict[str, Any]. Keep their existing behavior and docstrings unchanged, and ensure all four definitions satisfy the ANN202 requirement.Sources: Coding guidelines, Linters/SAST tools
🤖 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 `@aiopnsense/client_queue.py`:
- Around line 29-55: Update the docstrings for _do_get and _do_get_from_stream,
including the corresponding _do_post documentation, to describe immediate
transport/HTTP operations rather than queued requests. Keep their parameter and
return descriptions aligned with the underlying transport methods.
In `@aiopnsense/traffic.py`:
- Around line 100-102: Restore behavior-specific Google-style docstrings at all
listed sites: in aiopnsense/traffic.py lines 100-102, document the raw payload,
rate-calculation interval, and include_per_second_rates effect; at lines
203-206, describe interval clamping, endpoint selection, fallback timing, and
normalized samples yielded after the initial timing event; in
aiopnsense/helpers.py line 44, identify self as the instance whose _throw_errors
flag controls propagation; at lines 397-398, describe value normalization and
its expected string representation; and in docs/source/conf.py lines 75-80 and
109, document each Sphinx hook argument, the mutable lines list, and the
application used to register the hook.
In `@pyproject.toml`:
- Around line 175-187: Update the pydoclint configuration by changing
check-return-types from false to true so annotated return types are validated
against documented return types; leave the other [tool.pydoclint] settings
unchanged.
In `@tests/test_client_transport.py`:
- Around line 985-997: Update the _FakeIncrementalDecoder.decode stub to rename
the unused data parameter to _data, and change its docstring Args entry
accordingly; leave the final-based decoding behavior unchanged.
In `@tests/test_firewall.py`:
- Around line 36-41: Replace broad Any annotations across tests/test_firewall.py
(lines 36-41, 72, 286-291, 536-541, 581, and 608), tests/test_speedtest.py
(lines 19-24, 40, 173, 208, 244, 288, 446, 469, and 490), tests/test_vpn.py
(lines 18-24, 59, 85, 114, 209, 226, 256, 268, 436, 447, 537, and 566), and
tests/test_vnstat.py (lines 13-18, 43, 59, 101, 124, 234, 266, 286, and 356)
with precise fixture, factory, path, monkeypatch, payload, ClientType, or
MakeClientFactory annotations matching each test's parameters; retain Any only
where required at a genuine test boundary.
In `@tests/test_scripts_aiopnsense_dump.py`:
- Around line 666-678: Update fake_run_endpoint’s client_arg annotation from Any
to the local FakeClient type, and change the matching docstring type from Any to
FakeClient. Leave the endpoint behavior and other parameter annotations
unchanged.
In `@tests/test_scripts_opnsense_api_call.py`:
- Around line 133-152: Update FakeSession.get and FakeSession.post to annotate
their keyword arguments as **kwargs: object instead of Any, and document
**kwargs (object) in each method’s Args section when required by the project’s
linting configuration.
In `@tests/test_unbound.py`:
- Around line 47-52: Replace the Any annotations in tests/test_unbound.py at
lines 47-52, 85-92, and 370-386 with the shared ClientType and the specified
concrete types: ClientType for make_client; ClientType and object for
make_client and api_response; and ClientType, str, str | None, object, object,
and bool for the six parameters. Update each corresponding docstring type entry
to match, including tests/test_unbound.py line ranges 47-52, 85-92, and 370-386.
In `@tests/test_vpn.py`:
- Around line 114-119: Remove the unused make_client parameter from
test_wireguard_processing_and_updates and delete its corresponding Args
documentation entry, leaving the test behavior unchanged.
---
Outside diff comments:
In `@tests/test_vpn.py`:
- Around line 85-93: Update the fake_safe_list_get test helper by renaming its
unused path parameter to _path, adding the return annotation list[object], and
revising the Args documentation to use the new parameter name.
- Around line 59-67: Update each nested fake_safe_dict_get helper at the four
occurrences in tests/test_vpn.py to declare the return type as dict[str, Any].
Keep their existing behavior and docstrings unchanged, and ensure all four
definitions satisfy the ANN202 requirement.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 8d8cc08c-39dd-476b-b338-d743699a9dc2
📒 Files selected for processing (35)
aiopnsense/client_base.pyaiopnsense/client_endpoint.pyaiopnsense/client_queue.pyaiopnsense/client_transport.pyaiopnsense/helpers.pyaiopnsense/traffic.pyaiopnsense/vnstat.pydocs/source/conf.pyprek.tomlpyproject.tomlscripts/_opnsense_live_common.pyscripts/aiopnsense_dump.pyscripts/opnsense_api_call.pytests/conftest.pytests/test_client_base.pytests/test_client_endpoint.pytests/test_client_queue.pytests/test_client_transport.pytests/test_docs_conf.pytests/test_exceptions.pytests/test_firewall.pytests/test_firmware.pytests/test_helpers.pytests/test_nut.pytests/test_scripts_aiopnsense_dump.pytests/test_scripts_live_common.pytests/test_scripts_opnsense_api_call.pytests/test_smart.pytests/test_speedtest.pytests/test_system.pytests/test_telemetry.pytests/test_traffic.pytests/test_unbound.pytests/test_vnstat.pytests/test_vpn.py
💤 Files with no reviewable changes (3)
- tests/test_smart.py
- aiopnsense/vnstat.py
- tests/test_telemetry.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@aiopnsense/helpers.py`:
- Around line 398-407: Update the Returns section of the normalized API
comparison function’s docstring to declare the return type as bool:, while
preserving the existing description of the returned value.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: eba0df23-af01-4315-887a-1d2d087a0151
📒 Files selected for processing (20)
aiopnsense/client_queue.pyaiopnsense/helpers.pyaiopnsense/traffic.pyaiopnsense/vnstat.pydocs/source/conf.pypyproject.tomltests/conftest.pytests/test_client_endpoint.pytests/test_client_transport.pytests/test_firewall.pytests/test_helpers.pytests/test_scripts_aiopnsense_dump.pytests/test_scripts_live_common.pytests/test_scripts_opnsense_api_call.pytests/test_smart.pytests/test_speedtest.pytests/test_telemetry.pytests/test_unbound.pytests/test_vnstat.pytests/test_vpn.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds pydoclint to the prek checks and brings the repository's Google-style docstrings into compliance with the configured documentation contract.
What Changed
Why
Makes the existing documentation conventions enforceable in local and CI checks, so future API and test-helper changes keep their signatures and docstrings aligned.
Summary by CodeRabbit
Greptile Summary
The PR adds pydoclint to the repository’s prek checks and configures Google-style documentation validation. It updates docstrings and test callback annotations to align documented arguments, returns, yields, and exceptions with their signatures and behavior.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains; the previously reported traffic stream documentation issue is fixed in the current code.
Important Files Changed
Reviews (5): Last reviewed commit: "Clarify docstring descriptions" | Re-trigger Greptile