docs: add Google-style docstrings to public api/services/utils/models…#119
docs: add Google-style docstrings to public api/services/utils/models…#119clean6378-max-it wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (7)
📝 WalkthroughWalkthroughDocstrings are added to public functions and classmethods across ChangesDocstring additions across api/, models/, services/, and utils/
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🤖 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 `@api/composers.py`:
- Around line 131-139: The docstring for the function that fetches a composer by
ID (starting with "Fetch one composer by ID") has an inaccurate Returns section.
The current text states "404 when not found or schema drift blocks serving" but
schema drift is actually logged and skipped before attempting the global
fallback, so it does not block serving and is not a 404 condition. Update the
Returns documentation to accurately describe that a 404 only occurs when the
composer is not found in either per-workspace or global storage, and clarify
that schema drift issues are handled gracefully through logging and the fallback
mechanism.
In `@api/logs.py`:
- Around line 33-38: Update the docstring for the handler function to accurately
reflect the actual return format. The Returns section currently documents a bare
JSON array, but the handler actually wraps the log summaries in a logs object.
Change the documentation to specify that the response is {"logs": [JSON array of
log summary objects]} instead of just the array itself, so the docstring matches
the actual implementation behavior.
In `@api/pdf.py`:
- Around line 50-56: The docstring for the `generate_pdf()` function is
misleading about the request/response contract. Since the function defaults
missing `markdown` and `title` values instead of rejecting them, the docstring
should not describe them as required body parameters. Additionally, since the
function only emits 500 JSON errors and does not validate or return 400 errors,
the docstring should be updated to reflect only the actual error responses that
can occur. Revise the docstring to accurately describe that `markdown` and
`title` are optional with default values, and update the error documentation to
mention only 500 errors.
In `@api/workspaces.py`:
- Around line 169-179: The docstring for the tabs listing function is incomplete
and inaccurate. Update the Args section to document that `global` is also a
valid workspace_id value (in addition to storage folder names and
cli:<project_id>). Additionally, update the Returns section to clarify that the
helper function can return 200 with an empty tabs list ({"tabs": []}) even when
sqlite3.Error exceptions occur upstream, rather than only documenting 500 on
failure.
In `@models/cli_session.py`:
- Around line 20-30: The docstring for the Raises section in the function only
documents that SchemaError is raised when latestRootBlobId is missing or not a
string, but the require_truthy validation function also rejects falsey/empty
values (like empty strings, None, etc.) before the type check occurs. Update the
error description in the Raises section of the docstring to include falsey
values in the validation contract, so it accurately reflects all conditions
under which SchemaError is raised.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78b2c72f-f7b1-4177-99c2-07ca269199fb
📒 Files selected for processing (13)
api/composers.pyapi/config_api.pyapi/logs.pyapi/pdf.pyapi/search.pyapi/workspaces.pymodels/cli_session.pymodels/conversation.pymodels/export.pymodels/workspace.pyservices/summary_cache.pyutils/display_bubble.pyutils/workspace_path.py
|
@CodeRabbit, please review this PR in more depth |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr diff 119 --repo cppalliance/cppa-cursor-browserLength of output: 19055 🏁 Script executed: #!/bin/bash
# Check actual module path for validate_workspace_path
fd "path_validation.py" .
grep -rn "validate_workspace_path" --include="*.py" -l
# Check search type values in search.py
cat -n api/search.py
# Verify get_composer 404 behavior on schema drift
cat -n api/composers.py
# Check nocache env var truthy values in summary_cache.py
grep -n "CURSOR_CHAT_BROWSER_NOCACHE" services/summary_cache.py -A 5
# Check require_non_empty_str_fields call in export.py to verify documented fields
cat -n models/export.py
# Check CliSessionMeta indentation in docstring
cat -n models/cli_session.pyLength of output: 21084
Overall AssessmentThe intent is good and most docstrings are accurate. There are one confirmed indentation defect, four non-Google-style section headers (the main systemic issue), and several minor omissions. No functional code was changed, so the risk is low, but the issues below reduce the quality of the very documentation this PR is meant to improve. 1.
|
| File | Function | Non-standard header |
|---|---|---|
api/search.py |
search |
Query params: |
api/config_api.py |
set_workspace |
Body: |
api/workspaces.py |
get_workspace_tabs |
Query params: |
api/pdf.py |
generate_pdf |
Body (optional): |
The fix is to fold the request parameters into the standard Args: section (as is done for path parameters in get_workspace and get_workspace_tab) or into a Note: section if you prefer to keep them separate from code arguments:
# api/search.py — example fix
Args:
q: Search query string (required; 400 returned when empty).
type: Filter scope — ``all`` (default), ``chat``, or ``composer``.3. services/summary_cache.py — nocache_enabled: vague env-var description
The docstring says "True when bypass is requested or CURSOR_CHAT_BROWSER_NOCACHE is set" but the code (confirmed lines 40–42) checks for the values "1", "true", and "yes". Just saying "is set" implies any non-empty value works (e.g., "false" would not bypass, but a reader would think it does).
# Suggested
Returns:
True when bypass is requested or ``CURSOR_CHAT_BROWSER_NOCACHE``
is set to ``"1"``, ``"true"``, or ``"yes"`` (case-insensitive).4. api/search.py — search: 500 body also includes "results": []
The code at line 69 returns {"error": "Search failed", "results": []} on the 500 path (the results key is present even on failure). The docstring only documents the 400/500 statuses but omits the results: [] always present in error responses, which is a useful client contract to know.
5. api/workspaces.py — list_workspaces: nocache param not in Args:
list_workspaces has no Args: section, yet it honours ?nocache=1. Since get_workspace_tabs does document its query params (even if in a non-standard way), this creates an inconsistency. Suggest adding a minimal Args: section:
Args:
nocache: When ``1``, bypass the summary disk cache.6. api/composers.py — Missing blank line between route handlers (pre-existing, but visible in diff)
The diff shows the closing line of list_composers and the @bp.route decorator for get_composer on consecutive lines with no blank line between them. PEP 8 requires two blank lines between top-level definitions. This is a pre-existing issue exposed by the diff context.
return json_response({"error": "Failed to get composers"}, 500)
# ← missing blank line
`@bp.route`("/api/composers/<composer_id>")What is Accurate ✓
These items were verified against the actual implementation and are correct:
get_composerreturning 404 (not 500) on global-storage schema drift (confirmed line 240)ExportEntry.from_dictrequired fieldslog_id,title,workspace(confirmed line 48):func:utils.path_validation.validate_workspace_path`` module path (fileutils/path_validation.pyexists)searchtype valuesall | chat | composer(matches module-level comment and branch logic)- Env-var name
CURSOR_CHAT_BROWSER_NOCACHEinnocache_enabled(confirmed lines 6–7 of module docstring and line 39) CliSessionMeta.from_dictRaisesdescription of the three failure modes (falsey, missing, non-string)
Summary of required fixes (prioritised):
| Priority | File | Issue |
|---|---|---|
| 🔴 Must | models/cli_session.py |
Raises continuation indentation |
| 🟡 Should | api/search.py, api/config_api.py, api/workspaces.py, api/pdf.py |
Replace Query params: / Body: with Args: |
| 🟡 Should | services/summary_cache.py |
Specify accepted env-var values |
| 🟢 Nice | api/search.py |
Document "results": [] in 500 body |
| 🟢 Nice | api/workspaces.py |
Add Args: to list_workspaces |
| 🟢 Nice | api/composers.py |
Add missing blank line between routes |
|
all deep review findings aree addressed @CodeRabbit? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the latest diff to check all flagged items
gh pr diff 119 --repo cppalliance/cppa-cursor-browserLength of output: 22456
The PR looks good to merge from a documentation quality standpoint. 🐇✨ |
| infer_workspace_name_from_context, | ||
| lookup_workspace_display_name, | ||
| ) | ||
| from services.workspace_resolver import infer_workspace_name_from_context |
There was a problem hiding this comment.
1): it is the cleanup your own PR body lists as out of scope, so it contradicts "docstrings only, not function changes"
2): #118 is still open and remove this exact same import, so whichever merges first makes the other conflict on this line. Recommend pulling this hunk out of #119 and letting #118 own the import cleanup
2 participants
Closes #108
Summary
Add Google-style docstrings to the remaining undocumented public surface across
api/,services/,utils/, andmodels/. Fills gaps in route handlers, summary-cache helpers, workspace path overrides, display-bubble accessors, and allfrom_dictclassmethods on core model types.Sprint item #5 (Tuesday PR 1 of 2, 3 pt). Docstrings only — no functional changes.
Problem
Public functions and model parsers across the web API and CLI export paths lacked parameter, return, and side-effect documentation, slowing onboarding and review.
Changes
api/ (6 files)
workspaces.py,search.py,composers.py,config_api.py,logs.py,pdf.pyPDFDocclass andheader/footermethods inpdf.pyservices/
summary_cache.py:nocache_enabledand all cache get/set helpersutils/
workspace_path.py:set_workspace_path_override,get_workspace_path_overridedisplay_bubble.py:display_bubble_metadata,display_bubble_tool_callsmodels/
from_dictonComposer,WorkspaceLocalComposer,Bubble,Workspace,CliSessionMeta,ExportEntryKey files that already had docstrings (
workspace_listing.py,workspace_resolver.py,export_api.py, etc.) were left unchanged.Out of scope
Test plan
mypy .— passpytest— 468 passed (7 PDF tests skip/fail locally withoutfpdfinstalled)pytest/ruffon reviewer environmentAcceptance criteria
api/,services/,utils/documented (summary, Args, Returns)from_dictclassmethods documentedmypygreenSummary by CodeRabbit