docs(chunker): document None option for include_delim#634
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request updates the docstring for the include_delim parameter in split_text_by_delimiters to document the None option. The reviewer identified that the None option is currently non-functional in the existing implementation and suggested reformatting the docstring to improve readability and align with PEP 257 standards.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| include_delim: Whether to include delimiters in the current chunk ("prev"), | ||
| the next chunk ("next"), or to drop them entirely (None). Defaults to "prev". |
There was a problem hiding this comment.
While this documentation change correctly reflects that include_delim can be None, this option appears to be non-functional in practice. All callers of split_text_by_delimiters in the repository (e.g., in SentenceChunker, SemanticChunker) use a pattern like include_delim=self.include_delim or "prev". This means that when include_delim is None, it will incorrectly default to "prev" instead of dropping the delimiters. Since this PR's goal is to document this feature, it's important that it works as described. It would be best to fix the callers to correctly handle the None case.
There was a problem hiding this comment.
Good catch — you're right that the class-level chunkers (SentenceChunker, SemanticChunker, RecursiveChunker) all use the self.include_delim or "prev" pattern, which silently coerces None back to "prev" before it ever reaches this function. So at the chunker level, "drop delimiters entirely" is currently unreachable, even though split_text_by_delimiters itself passes None through to chonkie_core.split_offsets correctly (and SentenceChunker.__init__ explicitly validates None as an accepted value).
Two ways to resolve the inconsistency — happy to go either way, maintainer's call:
- Keep this docstring as-is (it accurately documents the free function's contract) and fix the callers in a follow-up: change
self.include_delim or "prev"toself.include_delim(the constructors already default to"prev", so theorguard is redundant and only serves to swallow an intentionalNone). - Scope the docstring to what's reachable today: drop the
Nonemention here until the caller-side behavior is fixed.
I lean toward option 1 since SentenceChunker already advertises and validates None in its own API surface — the or "prev" coercion looks like the actual bug. But I don't want to widen a docs PR into a behavior change without a maintainer nod. If you'd like, I can open a separate small PR for the caller fix with a regression test.
| include_delim: Whether to include delimiters in the current chunk ("prev"), | ||
| the next chunk ("next"), or to drop them entirely (None). Defaults to "prev". |
There was a problem hiding this comment.
The updated docstring is not ideally formatted for readability. The second line is not correctly indented and the combined line is quite long. According to the PEP 257 style guide, continuation lines for a parameter description should be indented to align with the start of the description on the first line. I'd suggest reformatting to improve readability and adhere to common line length limits.
| include_delim: Whether to include delimiters in the current chunk ("prev"), | |
| the next chunk ("next"), or to drop them entirely (None). Defaults to "prev". | |
| include_delim: Whether to include delimiters in the current chunk ("prev"), | |
| the next chunk ("next"), or to drop them entirely (None). | |
| Defaults to "prev". |
References
- According to PEP 257, for multi-line docstrings, the subsequent lines of a parameter description should be indented to the same level as the first line of the description. (link)
There was a problem hiding this comment.
Done — split the continuation onto its own line in latest push. I kept the standard 4-space hanging indent (consistent with the other multi-line Args entries in this module) rather than aligning to the description start column, since that's the prevailing style in this file.
What
split_text_by_delimitersinsrc/chonkie/chunker/base.pyacceptsinclude_delim: Optional[str] = "prev", but its docstring only documented the "prev" and "next" options, omitting the validNone("drop delimiters entirely") case.The
Noneoption is a real, intended value:sentence.py:86-87enforces the contractif include_delim not in ["prev", "next", None]: raise ValueError("include_delim must be 'prev', 'next' or None"), and the sentence docstring describes it as the delimiters going in the "current chunk, next chunk or not at all".Fix
One-line docstring update to also document
None:Scope
Docs-only. No behavior change — a single docstring hunk in
base.py.Verified
Cross-referenced the accepted-values contract in
sentence.py:86-87(ValueError guard listingNone) and the sibling docstring wording.