fix: TeX false positive SentenceCapitalization on multi-line sentences with indentation - #3836
Conversation
|
This is partly nonsense. It should take into account the actual rules in TeX. These are (simplified): Let's call a character blank if it's space or tab.
I think these rules are good for a first PR, but there are further commonly used things that can be considered:
|
| #[test] | ||
| fn keeps_separator_for_blank_line() { | ||
| let source: Vec<_> = "word\n\nword".chars().collect(); | ||
|
|
||
| assert_eq!(allowed_text(&source), "word\nword"); | ||
| } | ||
|
|
There was a problem hiding this comment.
| #[test] | |
| fn keeps_separator_for_blank_line() { | |
| let source: Vec<_> = "word\n\nword".chars().collect(); | |
| assert_eq!(allowed_text(&source), "word\nword"); | |
| } | |
| #[test] | |
| fn keeps_separator_for_empty_line() { | |
| let source: Vec<_> = "word\n\nword".chars().collect(); | |
| assert_eq!(allowed_text(&source), "word\nword"); | |
| } | |
| #[test] | |
| fn keeps_separator_for_blank_line() { | |
| // "Blank" characters are space and tab | |
| let source: Vec<_> = "word\n\t \t \nword".chars().collect(); | |
| assert_eq!(allowed_text(&source), "word\nword"); | |
| } | |
| #[test] | |
| fn collapses_blank_lines() { | |
| let source: Vec<_> = "word\t \n\t \t \n \n\n\n\t\n \tword".chars().collect(); | |
| assert_eq!(allowed_text(&source), "word\nword"); | |
| } |
| #[test] | ||
| fn does_not_mask_spaces_within_sentence() { | ||
| // Spaces between words (not before %) must remain in allowed spans | ||
| // so that lints like double-space detection can still fire. | ||
| let source: Vec<_> = "word word".chars().collect(); | ||
| let mask = Masker::default().create_mask(&source); | ||
| let allowed: String = mask | ||
| .iter_allowed(&source) | ||
| .flat_map(|(_, chars)| chars.iter().copied()) | ||
| .collect(); | ||
| assert_eq!(allowed, "word word"); | ||
|
|
||
| assert_eq!(allowed_text(&source), "word word"); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is precisely the wrong thing to do. In TeX, it doesn't matter if there's a single space or multiple spaces.
| #[test] | |
| fn does_not_mask_spaces_within_sentence() { | |
| // Spaces between words (not before %) must remain in allowed spans | |
| // so that lints like double-space detection can still fire. | |
| let source: Vec<_> = "word word".chars().collect(); | |
| let mask = Masker::default().create_mask(&source); | |
| let allowed: String = mask | |
| .iter_allowed(&source) | |
| .flat_map(|(_, chars)| chars.iter().copied()) | |
| .collect(); | |
| assert_eq!(allowed, "word word"); | |
| assert_eq!(allowed_text(&source), "word word"); | |
| } | |
| #[test] | |
| fn collapses_spaces_within_sentence() { | |
| let source: Vec<_> = "word word".chars().collect(); | |
| assert_eq!(allowed_text(&source), "word word"); | |
| } |
| #[test] | ||
| fn collapses_newline_indentation_to_single_space() { | ||
| let source: Vec<_> = "word\n word".chars().collect(); | ||
|
|
||
| assert_eq!(allowed_text(&source), "word word"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Indentation is in no way special, so let's have a more general test:
| #[test] | |
| fn collapses_newline_indentation_to_single_space() { | |
| let source: Vec<_> = "word\n word".chars().collect(); | |
| assert_eq!(allowed_text(&source), "word word"); | |
| } | |
| #[test] | |
| fn collapses_whitespace_within_sentence() { | |
| let source: Vec<_> = "word\t \t\t \n\t\t word".chars().collect(); | |
| assert_eq!(allowed_text(&source), "word word"); | |
| } |
Implement the TeX whitespace model from the review: a run of blank-or-newline containing a blank (empty) line is a paragraph boundary and is preserved; any other run of blank-or-newline collapses to a single space so a sentence wrapped across lines reads as one sentence. Handle explicit spaces (\ ), non-breaking spaces (~), and line breaks (\\) as non-collapsing separators. Update masker tests and the issue_3224 fixture. Fixes Automattic#3224
68409bb to
b04c31f
Compare
|
Done - reworked the masker to follow the TeX whitespace model you laid out:
Rebased onto master; cargo build/test/fmt for harper-tex are green. I kept this to the two core rules plus the explicit/non-breaking spaces you flagged - happy to add the |
Filter ParagraphBreak tokens whose masked span is whitespace-only so multi-line TeX blocks stop triggering SentenceCapitalization false positives.
@mvanhorn is a known PR spam bot. I am considering banning him from opening new PRs, since most of his tend to be slop. |
|
Yeah, okay, I mean it was obvious that this is AI slop. If you still think that the current state of the PR is something you may want to build on, then let me know and I can have another look. |
dude I am literally NOT a PR spam bot. Had a call with Pedraum Pardehpoosh and a few others from Automattic team last week... how can I help? would love feedback / advice on my contributions, which are heavily led by AI but I am not a bot nor is this spam I genuinely want to help make Harper better. |
4 participants
Issues
Fixes #3224
Description
Change
newline_whitespace_at_cursorso that a newline followed by indentation collapses to a single visible whitespace character instead of being masked out entirely: mask the newline plus all-but-one of the following whitespace characters (returnws_len - 1rather thanws_len), leaving exactly one whitespace char allowed. This keeps the two wrapped lines joined by a single space, so no false sentence boundary is created, while still preventing multi-space indentation from tripping double-space lints (the original intent of the masking added in #3106). Keep the existing bare-newline path (ws_len == 1returnsNone, newline stays visible) unchanged, and never returnSome(0)so the masker cursor cannot stall. Add an inline masker unit test plus an integration fixture using the exact example from the bug report.Demo
N/A - behavioral fix; validated by the tests below.
How Has This Been Tested?
\begin{abstract}with a two-line indented sentence ending in a period) produces 0 lints via a newharper-tex/tests/test_sources/issue_3224.texfixture registered inrun_tests.rs.\itemstill does not leak a multi-space whitespace-only span (existingmasks_indentation_before_itemtest continues to pass).\nwith no indentation remains unflagged (unchanged behavior).\n\n) still leaves a visible separator so genuine paragraph/sentence boundaries are preserved.AI Disclosure
If Your PR Implements or Enhances a Linter
Checklist