Fix output correctness issues found in review - #2
Merged
IvoLeist merged 1 commit intoAug 5, 2026
Merged
Conversation
Input handling: - Decode the input as UTF-8 instead of Path.read_text(), which silently rewrote CRLF/CR line endings and shifted the reported start indices. - Report invalid UTF-8 input with a clear message instead of a traceback. - Reject input that is empty or whitespace only instead of writing empty outputs. TSV output: - Escape backslash, tab, carriage return and newline so that the chunk text stays on one row and the escaping can be reversed. The previous escaping could not be undone, and csv.writer additionally quoted any chunk containing a double quote. Errors and metadata: - Report a disallowed special token with a clear message on the character splitter as well, not only on the token splitter. - Report an invalid start index as null instead of replacing the number with a string, so the JSON field keeps a single type. The warning on stdout is unchanged. - Use a single "length" key per chunk together with the existing "length_unit" instead of character_count/token_count. - Report strip_whitespace as false in token mode, where the splitter never applies it. - Write all outputs as UTF-8 instead of the system locale encoding. Command line and separators: - Pass a custom separator as --separator=VALUE so that a separator starting with a dash is not read as an option. - Reject unsupported and out-of-range custom separator escapes with a clear message instead of crashing or passing them through silently. - Make --encoding-name and --model-name mutually exclusive. Tests and docs: - Add tests for whitespace-only input, a disallowed special token on the character splitter, a dash-prefixed custom separator, and a predefined separator inside the repeat. - Expect null start indices in the two overlapping token tests, which hit the same upstream bug as the dedicated repro test. - Document the tiktoken encoding cache, the TSV escaping and the JSON fields in the tool help. - Deduplicate the two tiktoken command line blocks into a macro.
IvoLeist
merged commit Aug 5, 2026
b46322d
into
IvoLeist:add-langchain_text_splitters
6 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The fixes for the points from my review on bgruening#1947, as discussed there.
Output correctness
Path.read_text(), which silently rewrote CRLF/CR line endings and shifted the start indices.\,\t,\rand\nand write the row without thecsvmodule, so each chunk stays on one row and the escaping can be reversed.csv.writeradditionally quoted every chunk containing a".Errors
--separator=VALUE, so a separator starting with a dash (e.g.---) is not read as an option.Output format
start_indexisnullwhen the splitter returns an invalid position, instead of replacing the number with a message string. The warning on stdout is unchanged and still carries the value.lengthkey per chunk together with the existinglength_unit, instead ofcharacter_count/token_count.strip_whitespaceis reported asfalsein token mode, where the splitter never applies it.Tests and docs
start_index: nulland the warning, instead of thenot_has_textcheck that only passed because the old code wrote a string there. They stay green now and will fail once the upstream bug (TokenTextSplitterstart indices are sometimes -1 langchain-ai/langchain#29884) is fixed, so nothing has to be re-enabled by hand.planemo lintclean.planemo test16/16 before, 20/20 after.