Repository navigation
Conversation
pszymkowiak
left a comment
There was a problem hiding this comment.
Tested extensively with the compiled binary. The targeted fix works correctly: a docstring that opens and closes on the same line ("""Return the sum.""") no longer corrupts state — confirmed with single and sequential occurrences, plus the empty-docstring edge case.
However, the same class of bug (from #2712) still reproduces for an equally common style: a multi-line docstring whose closing """ is appended directly to content rather than sitting alone on its own line — e.g.:
def subtract(self, a, b):
"""Subtract b from a
and return result."""
# subtract comment
return a - b
def multiply(self, a, b):
# multiply comment, no docstring before
return a * bThis outputs both # subtract comment and # multiply comment unstripped — the second one belongs to a completely different, docstring-less method, showing the corruption persists across the rest of the file exactly like the original bug, just triggered by a different closing style. Root cause: the fix only handles the open+close-on-one-line case; a closing """ line whose trimmed text doesn't start with """ never reaches the toggle logic and falls straight into the unconditional if in_docstring { push verbatim } path, so in_docstring never resets. It happens to "self-heal" the next time another line starts with """ (regardless of whether that's semantically related), but if none follows, corruption persists to EOF — as in this example.
Given the docstring-with-trailing-close style is arguably more common in real Python code than the single-line case this PR targets, I'd consider #2712 still open rather than fully fixed. cargo fmt, cargo clippy --all-targets, cargo test --all are otherwise clean (2 unrelated pre-existing locale failures).
|
Pushed a fix addressing the multi-line docstring closing case — the |
9eb14f7 to
4ef5d1d
Compare
|
Thanks for the thorough testing @pszymkowiak — the case you found (multi-line docstring whose closing |
Single-line docstrings like """Return the sum.""" toggled in_docstring to true but never back to false, causing all subsequent lines to be treated as docstring content. Comments after such docstrings were not stripped, reducing token savings. Check for closing """ on the same line after opening to correctly reset the state. Fixes rtk-ai#2712
The single-line fix only reset in_docstring when the line started with """. A closing """ appended to content (`and return result."""`), or one followed by trailing text on the same line, never reached the toggle, so in_docstring stayed true and every following line — including comments in unrelated, docstring-less functions — was emitted verbatim to EOF. Count the """ occurrences on the line instead of matching a prefix or a suffix: an odd count flips the state, which covers opening, closing, open-and-close-on-one-line and the empty """""" docstring with one rule. Tests cover the single-line, multi-line-closing-on-content-line, closing-with-trailing-text, sequential and empty-docstring cases.
4ef5d1d to
74a5cbd
Compare
|
@pszymkowiak rebased on current develop and ready for another look. Your repro (a multi-line docstring whose closing To re-check by hand, use a real Two things I noticed while poking at it, both pre-existing and unrelated to this PR: Heads-up: #2715 edits the same function (adjacent hunks), so whichever merges second needs a mechanical rebase. |
|
Thank you for this fix and the thorough test cases. #3336 (merged 2026-09-26) gave Python its own triple-quote-aware path in the minimal filter, so the docstring toggle this PR edits is no longer reached for Python files. All five of your shapes (one-line, closed on a content line, text after the closing quotes, sequential, empty) are now handled correctly on develop. Only the one-line case is pinned by a test there, so a small test-only PR adding the other shapes against the current code would be welcome, as would one removing the now-unused Python branch in the generic loop. If I missed a case, please comment or reopen. |
Summary
Fixes #2712
MinimalFiltercorrupted itsin_docstringstate on Python docstrings whose closing"""isn't on a line of its own, so comment stripping went wrong for the rest of the file. The state is now driven by one rule: count the"""on the line, and an odd count flips the state. That covers:"""Return the sum.""")"""appended to content (and return result.""")"""with trailing text (""" # note)""""""docstringHistory of this PR: the first commit handled the single-line case. After pszymkowiak's review, the second commit handled a closing
"""appended to content (his repro). Re-testing found the same failure one closing style further out (text after the closing quotes), so the second commit now replaces both special cases with the odd-count rule, which is net shorter than what is on develop.Verified
pszymkowiak's exact repro (
rtk read <file>.py -l minimal), develop vs this PR: on develop both# subtract commentand# multiply commentare left unstripped; here both are stripped, the docstring is kept and the following method is unaffected. Single-line, empty and sequential docstrings are correct.To try it by hand use a real
.pyfile:rtk read - -l minimalfrom stdin resolves toLanguage::Unknown, so no Python handling runs.Tests
Five docstring tests (single-line, closing on a content line, closing with trailing text, sequential single-line docstrings, empty docstring); all five fail on develop's code.
cargo fmt --all -- --check,cargo clippy --all-targets(0 warnings),cargo test --all(all pass)