Enforce public API docs inventory - #458
Conversation
📝 WalkthroughWalkthroughThe change adds runtime public API discovery and documentation coverage checks, documents Chart and Selection APIs, and introduces a local documentation verification workflow separate from example checks. ChangesAPI inventory and documentation checks
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Makefile
participant verify_local
participant verify_docs_local
participant DocsApp
Makefile->>verify_local: run docs check
verify_local->>verify_docs_local: invoke documentation verifier
verify_docs_local->>DocsApp: run docs tests and quality checks
DocsApp-->>verify_docs_local: return status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThe PR centralizes the supported component and method inventory, checks API-reference coverage, and separates documentation verification from example checks.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| scripts/check_public_api.py | Centralizes API discovery and manifest comparison, then validates public Chart and Selection methods against API-reference documentation. |
| scripts/verify_docs_local.py | Adds the documentation verification sequence covering dependency sync, docs tests, API inventory, quickstart validation, formatting, linting, and spelling. |
| scripts/verify_local.py | Registers the new documentation verification sequence as the docs check group. |
| tests/test_public_api.py | Reworks checker fixtures around discovered API categories and adds manifest and documentation-coverage tests. |
| tests/test_type_surface.py | Replaces duplicated API lists with the centralized inventory and extends type-surface coverage to support factories and Selection rows. |
| docs/api-reference/figure-methods.md | Documents notebook display behavior and the public chart view and selection state methods. |
| docs/api-reference/events-and-callbacks.md | Adds Selection.rows(limit=None) to the documented selection API. |
Reviews (4): Last reviewed commit: "Fix test helper import ordering" | Re-trigger Greptile
![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
🤖 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 `@scripts/check_public_api.py`:
- Around line 408-409: In the validation flow around build_public_api_inventory
and validate_declarative_api_contract, validate components_module.Chart and
__all__ before constructing the inventory. Return or otherwise stop inventory
construction when either guard reports invalid input, preserving the validation
errors instead of allowing dereference or getattr exceptions; add regression
coverage for a missing Chart and a non-string __all__ member.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da885517-cebc-4d69-b52c-a3f64e365bc7
📒 Files selected for processing (8)
Makefiledocs/api-reference/events-and-callbacks.mddocs/api-reference/figure-methods.mdscripts/check_public_api.pyscripts/verify_docs_local.pyscripts/verify_local.pytests/test_public_api.pytests/test_type_surface.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Tip: instead of fixing issues one by one fix them all with cubic
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Hi @masenf, could you please review PR when you have time? I’d appreciate feedback on the overall approach and any required changes. |
8f45166 to
5c2294b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/check_public_api.py (1)
490-503: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe fallback regex makes the doc check permissive.
_has_doc_referencereturnsTruewhen the document containsname(anywhere, without a receiver or backticks. Generic method names such asfigure,text,select,append,html, andshowthen match unrelated prose, code samples for other objects, or Python builtins. That weakens the coverage guarantee this PR adds.Consider requiring a qualified or code-formatted token, and keep the bare-name regex only as an explicit opt-in.
♻️ Suggested tightening
def _has_doc_reference(text: str, name: str, *, receiver: str | None = None) -> bool: tokens = [f"`{name}`", f"`{name}()`", f"`{name}(`"] if receiver is not None: tokens.extend( ( f"`{receiver}.{name}()`", f"`{receiver}.{name}(`", f"{receiver}.{name}(", ) ) - return ( - any(token in text for token in tokens) - or re.search(rf"(?<![A-Za-z0-9_]){re.escape(name)}\s*\(", text) is not None - ) + if any(token in text for token in tokens): + return True + if receiver is not None: + return False + return re.search(rf"(?<![A-Za-z0-9_.]){re.escape(name)}\s*\(", text) is not None🤖 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 `@scripts/check_public_api.py` around lines 490 - 503, Tighten _has_doc_reference so bare name( regex matches are not used by default, requiring receiver-qualified or backtick-formatted references instead. Add an explicit opt-in parameter for callers that genuinely need bare-name matching, and preserve the existing qualified-token checks.tests/test_public_api.py (1)
264-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe fake modules still pull in the real
xypackage.
fakeis named"xy", sobuild_public_api_inventoryrunsimportlib.import_module("._figure", pkg.__name__)and imports the realxy._figure. The declarative contract tests therefore validate the realSelectionsurface and the realdocs/api-referencefiles, not the fixtures defined here. A change to the installed package or to the docs can fail these tests for reasons unrelated to the fixture.Add a
figure_module(orselection_class) parameter tobuild_public_api_inventoryinscripts/check_public_api.py, and pass a fakeSelectionfrom this fixture.🤖 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_public_api.py` around lines 264 - 309, Update build_public_api_inventory in scripts/check_public_api.py to accept an optional figure_module or selection_class dependency and use it instead of importing the real xy._figure module. In the fixture setup, define a fake Selection and pass it through that parameter so the declarative contract tests inspect only the fake module surface and fixture-owned documentation.
🤖 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.
Nitpick comments:
In `@scripts/check_public_api.py`:
- Around line 490-503: Tighten _has_doc_reference so bare name( regex matches
are not used by default, requiring receiver-qualified or backtick-formatted
references instead. Add an explicit opt-in parameter for callers that genuinely
need bare-name matching, and preserve the existing qualified-token checks.
In `@tests/test_public_api.py`:
- Around line 264-309: Update build_public_api_inventory in
scripts/check_public_api.py to accept an optional figure_module or
selection_class dependency and use it instead of importing the real xy._figure
module. In the fixture setup, define a fake Selection and pass it through that
parameter so the declarative contract tests inspect only the fake module surface
and fixture-owned documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4043e4b5-ddb3-4a1f-bfd8-d6e8dc837ac3
📒 Files selected for processing (6)
docs/api-reference/figure-methods.mdscripts/check_public_api.pyscripts/verify_docs_local.pytests/_public_api_test_utils.pytests/test_public_api.pytests/test_type_surface.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api-reference/figure-methods.md
- tests/test_type_surface.py
1 participant
Closes #444
Summary
Testing
Summary by CodeRabbit
New Features
Selection.rows(limit=None), enabling access to selected rows with optional result limits.Documentation
Tests