docs: Harden Python docstring validation - #121
Conversation
| `(size, count)` pair meaning `count` consecutive chunks of `size`.""" | ||
| """One run of chunks along a rectilinear chunk edge. | ||
|
|
||
| Give a single chunk size, or a `(size, count)` pair. The pair means `count` |
There was a problem hiding this comment.
I don't like these as imperative mood because it's not a method, it's a type on an object. So these should be explaining a noun.
| Give a single chunk size, or a `(size, count)` pair. The pair means `count` | |
| A single chunk size, or a `(size, count)` pair. The pair means `count` |
There was a problem hiding this comment.
Fixed. Both type alias docstrings are noun phrases now — _RunLength uses your suggestion verbatim, and _ChunkEdgeLengths became "An integer if the dimension has one chunk size, or a sequence of runs if the chunk sizes change along the dimension."
Added the general rule to CLAUDE.md so this holds for the remaining 17 files: imperative verb for functions and methods, noun phrase for type aliases, classes, attributes and properties.
|
|
||
| Args: | ||
| array_shape: The shape of the array, in elements along each dimension. | ||
| chunk_shape: The shape of each chunk, in elements along each dimension. |
There was a problem hiding this comment.
I don't think this is the shape of each chunk? I think this is the shape of all chunks?
Check the upstream zarrs docstring
There was a problem hiding this comment.
Checked upstream — your original read was right, and the singular parameter name actually supports it rather than undermining it.
In zarrs 0.23.13, RegularChunkGrid stores a single chunk_shape and its accessor takes no chunk indices, so every chunk shares one shape:
// regular.rs
pub fn chunk_shape(&self) -> &[NonZeroU64] { self.chunk_shape.as_slice() }
// regular_bounded.rs
fn chunk_shape(&self, chunk_indices: &[u64]) -> Result<Option<ChunkShape>, _>chunk_shape is singular precisely because all chunks share it. Changed to "The shape of all chunks".
regular_bounded is the opposite case and I had it wrong — it takes chunk indices and clamps to the array bounds, so chunk shapes genuinely differ there. That one now reads "The maximum shape of a chunk ... The grid clips the chunks at the array bounds to a smaller shape."
| - **Write all documentation in ASD-STE100 Simplified Technical English.** This | ||
| applies to docstrings, `.pyi` stubs, Rust doc comments, Markdown pages, and the | ||
| README. Use one topic per sentence, keep sentences short (20 words or fewer for | ||
| descriptive text, 20 for instructions), use the active voice, use the approved |
There was a problem hiding this comment.
Use the active voice for functions and methods
There was a problem hiding this comment.
Added as its own bullet, since it needs to say what the other case is too:
Use the active voice for functions and methods, and a noun phrase for everything else. A function or method docstring starts with an imperative verb: "Construct a regular grid...", "Return the store key...". A type alias, class, attribute, or property docstring names what the thing is: "The chunk sizes along one dimension."
Also fixed the sentence-length line you commented on — it said "20 words or fewer for descriptive text, 20 for instructions", which was both self-contradictory and wrong. STE is 20 max for procedural sentences, 25 for descriptive.
Add Args/Returns/Raises sections to the small stub modules and the codec modules, and apply the Simplified Technical English rules to their prose. Every Raises entry is verified by calling the built extension rather than inferred from the Rust source. That turned up several behaviours the old docstrings did not record: - gzip validates its level range but zstd does not; a negative level gives OverflowError rather than ValueError. - blosc raises PluginCreateError (not ValueError) when shuffling is enabled without a typesize. - FilesystemStore does not check that its path exists at construction. - ArrayBytes does not check mask or offsets against the data buffer. Also disable D413, which required a blank line after the final docstring section. That is not Google style, and it is a rule that setting a pydocstyle convention would have silently disabled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… group Add Args/Returns/Raises sections and apply the Simplified Technical English rules to the prose in these four stub modules. Verified against the built extension rather than inferred. Notable findings: - Group.open raises GroupCreateError for missing metadata but ValueError for a path that is not absolute; it does not use NodePathError. - Group.child and Group.__getitem__ raise KeyError, not a zarrista exception. - Tensor.__buffer__ raises BufferError only for PyBUF_WRITABLE; the previous wording was correct but untested. - VariableArray.__arrow_c_array__ raises TypeError if requested_schema is neither None nor a PyCapsule. - The existing "succeeds if it does not exist" claim on erase_metadata holds; calling it twice is fine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Complete the Google-style rewrite with _builder.pyi and _array.pyi, then turn on the pydoclint settings that keep it that way: - skip-checking-short-docstrings = false requires Args/Returns on every non-property method. The stubs are at zero violations under it. - require-inline-class-var-docs = true keeps CodecOptions documenting its members inline, which is what mkdocstrings renders best. - skip-checking-raises = true because stub bodies are `...`, so pydoclint can never see a raise statement and would reject every Raises section. Raises entries come from calling the built extension. That recorded several behaviours the previous "Raises if ..." wording left unnamed, and some that contradict the obvious guess: - Array.open raises ArrayCreateError for missing metadata but ValueError for a path that is not absolute. - chunk_origin/chunk_shape/chunk_subset raise ArrayError for a wrong number of dimensions, but accept out-of-bounds indices. - chunk_key validates nothing and returns a key for indices outside the grid. - On a read-only array, store_chunk raises ArrayError while erase_chunk, erase_metadata and store_metadata raise StorageError. - Unsupported indexing raises NotImplementedError (step != 1, np.newaxis) and too many indices raises IndexError. - An unknown codec option raises TypeError. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rewrite the README, DEVELOP.md and the DecodedArray page in short active sentences with one topic each, and keep the Zarr domain terms. Fix two errors in the README example, both confirmed by running it: - Indexing was documented as returning a `Data` buffer. No such type exists; a read returns a `DecodedArray`, so link to that instead. - The `array.dtype` output comment read `DataType(float32)`. The actual repr is `DataType(float32 / <f4)`. Also re-sync the `AsyncStore` and `DecodedArray` docstrings in the `.py` files with the rewritten `.pyi` stubs, which had drifted apart, and record the new pydoclint requirements in DEVELOP.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes #120