Document parameters missing from the API reference - #861
Conversation
Every function here has arguments in its signature that its Parameters section never listed, so the rendered reference page shows a signature and a parameter table that disagree. Most are the leading `patch` argument, which made the table look shifted by one: `write` starts at `path` while its signature starts at `patch_or_spool`. The convention in the package is to document it -- 39 proc functions already do -- so these were simply omissions. The rest are ordinary gaps: scan_to_df's pass-through ext/timestamp/ progress, get_coord's `max`, pad's `samples`, drop_coords' `array`, get_slice_tuple's `relative`, and spool_to_directory's `extension`. BaseSpool.select ran the other way: it documented the four keyword-only arguments the implementation takes while the abstract signature declared only **kwargs, which is also what griffe warns about on every doc build. Give the abstract method the signature its docstring already describes.
|
Warning Review limit reached
Next review available in: 6 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
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 |
`relative` resolves against the coordinate's minimum and maximum, not its start and stop, which differ for a reversed coordinate. `max` is an exact alias for the exclusive `stop`, so get_coord(min=0, max=10, step=1) yields a coordinate whose maximum is 9. drop_coords empties the axes of dropped coordinates to length zero rather than removing them, so the array keeps its dimensionality.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0334bf0109
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _attrs: dict | None = None, | ||
| _coords: dict | None = None, |
There was a problem hiding this comment.
When a caller uses the documented tagging forms such as _attrs="tag" or _coords=["sensor"], the implementation accepts them (and the selection-spec tests exercise both), but this public signature declares only dictionaries. Because dc.spool() and several readers are typed as returning BaseSpool, static type checkers will reject these valid API calls; annotate each parameter to include a name and a collection of names as well as the mapping form.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 103ef0e. Confirmed the tag forms are exercised by the selector-spec tests (_attrs="tag", _coords=["sensor"]) and accepted by resolve_selector_namespaces, which normalizes a str or collection alongside the mapping form. Added a namespace_select_type alias in constants and applied it to BaseSpool.select, Spool.select, PatchCatalog.select and the resolver itself, so the whole chain admits what the docstring promises.
| relative | ||
| If True, the select values are relative to the start (positive) |
There was a problem hiding this comment.
For a descending coordinate, start and stop are the first and last endpoints, but _get_relative_values always resolves nonnegative values from self.min() and negative values from self.max(). Consequently, this description promises the wrong endpoints for reverse-sorted coordinates; for example (2, -2) selects from min + 2 through max - 2, not offsets from the coordinate's start and stop. Document these as offsets from the minimum and maximum instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e44d314. Verified: _get_relative_values resolves nonnegative values from self.min() and negative from self.max(), so the description now says minimum/maximum rather than start/stop.
| The stopping value of an array, exclusive. | ||
| max |
There was a problem hiding this comment.
Calling this parameter the maximum value is incorrect because the implementation assigns it directly to the exclusive stop. For example, get_coord(start=0, max=10, step=1) contains values only through 9, and on descending ranges the value is not a maximum at all. Describe max solely as an alias for the exclusive stop bound so callers do not introduce an off-by-one error when constructing coordinates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e44d314. Confirmed empirically that get_coord(min=0, max=10, step=1) equals the start/stop form and has a maximum of 9, so max is now described as an alias for the exclusive stop.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #861 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 176 176
Lines 19372 19276 -96
=========================================
- Hits 19372 19276 -96
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`_attrs` and `_coords` take either a mapping of name -> selector or a name/collection of names tagging bare kwargs, which is what the docstring describes and what resolve_selector_namespaces implements; the annotations admitted only the mapping, so a type checker rejected the documented `select(tag="some*", _attrs="tag")` form. Name the accepted shape once in constants and use it everywhere the selectors are passed down.
1 participant
Description
Several public functions have arguments in their signature that their
Parameterssection never documented, so the rendered API reference shows a signature and a parameter table that disagree.dc.writeis the clearest example: the signature iswrite(patch_or_spool, path, file_format, ...)while the table starts atpath, which reads as though every description is shifted by one argument.Twelve of these are the leading
patch/patch_or_spool/patchesargument. That is an omission rather than a house style — 39 of theprocfunctions that takepatchfirst already document it, and 12 did not.The remaining gaps are ordinary ones:
scan_to_df—ext,timestampandprogress(passed through todc.scan, and documented there)get_coord—max(min,startandstopwere documented)Patch.pad—samplesCoordManager.drop_coords—arrayBaseCoord.get_slice_tuple—relativeexamples.spool_to_directory—extensionOne case ran the other way.
BaseSpool.selectdocuments the four keyword-only arguments the implementation accepts (_attrs,_coords,samples,relative), but the abstract method declared only**kwargs— the mismatch griffe reports asspool.py:295: Parameter '_attrs' does not appear in the function signatureon every doc build. The abstract signature now matches the docstring andSpool.select; subclasses are unaffected at runtime.No behavior changes: docstrings plus one abstract signature.
Found by auditing every callable in the documented API with
inspect.signatureagainst its numpydocParameterssection; the audit now reports zero mismatches.Checklist
I have (if applicable):