Skip to content

check(sequence): add max/min count threshold, same as occurrence - #1162

Draft
theredspoon wants to merge 1 commit into
vale-cli:v3from
theredspoon:feat/sequence-max-min
Draft

check(sequence): add max/min count threshold, same as occurrence#1162
theredspoon wants to merge 1 commit into
vale-cli:v3from
theredspoon:feat/sequence-max-min

Conversation

@theredspoon

Copy link
Copy Markdown

Refs #1161. Draft, not requesting review yet: the change works and is tested, but it does not fully solve the motivating use case in the issue, explained below, and I want that visible before anyone spends review time on it.

What this adds

Max/Min int on Sequence, same names and semantics as Occurrence's. Unset (the zero value, every existing sequence rule today) keeps current behavior exactly, one alert per match. Setting either buffers matches instead of alerting per match, and after the scope's candidates are exhausted, fires once with the match count substituted into the message via core.CondSprintf, the same formatter occurrence already uses for this.

Also fixes a real bug the change surfaced: the existing literal-filter optimization returns before tagging or matching whenever the rule's required literal is absent from the block, correct for a normal rule, wrong for min, whose whole point is to detect too few matches, including zero. Skipped now specifically when Min is set.

What I verified

Unit tests following the existing table-driven convention in sequence_test.go: max firing once with the right count on repeated matches, staying silent under the threshold, min firing the document-scoped fallback on zero matches (mirroring occurrence's own zero-match branch), and confirming every existing sequence rule with Max/Min unset behaves identically to before.

go test on every non-e2e package, golangci-lint run (v2.5, matching CI), gofmt -l, all clean. Did not run the e2e suite, it needs a toolchain (Ruby, Java, dita-ot, typst2vast, etc.) I don't have set up here; the change doesn't touch anything e2e would exercise, but flagging the gap rather than silently skipping it.

Built the actual binary and ran a real VerbTricolonDensity-shaped rule (max: 1, the tricolon pattern from #1161) against real text, not just unit tests:

  • Two tricolons packed into one long sentence: fires once, correct count in the message.
  • One tricolon per sentence, two sentences in the same paragraph: no alert.

The real limitation

That second case is the actual motivating scenario in #1161, and it doesn't work. Root cause, confirmed by reading internal/nlp/provider.go's doNLP and sequence.go's sentenceScope: a sequence rule's declared scope is unconditionally narrowed to sentences in NewSequence, regardless of what the YAML declares or whether Max/Min is set. Each sentence gets its own independent Run call with no state shared between them, confirmed empirically too: the plain unbounded version of the same rule, no Max/Min at all, already produces two separate alerts (one per sentence) for that same two-sentence input, which is what makes the Max/Min counting invisible across sentences, there's nothing to count across, each call only ever sees its own sentence's matches.

Closing that gap would need two more things I did not attempt here: relaxing sentenceScope's narrowing specifically for a rule that sets Max/Min (tagging accuracy doesn't require it, f.TokensWith already segments and tags per sentence internally regardless of the input block's size, so this looks safe on that front), and a guard against a match spanning a sentence boundary once multiple sentences are actually reachable in one Run call, to avoid a new correctness problem. Given sequence's scope handling has already had two real, subtle bugs found and fixed very recently (#1124, #1126), I'd rather flag this precisely than rush a second change into the same area under one PR.

Happy to take this further if a sentenceScope relaxation along these lines looks like the right direction to you, or to narrow the issue to just the single-sentence case this PR actually delivers if that's more useful on its own.

Adds Max/Min int fields to Sequence, mirroring Occurrence's fields
and semantics: unset (the zero value) keeps every existing sequence
rule's behavior of one alert per match, unchanged. Setting either
turns the rule into a density check, buffering matches instead of
alerting per match and, after the scope's candidates are exhausted,
firing once with the match count substituted into the message via
core.CondSprintf, the same formatter occurrence already uses for
this.

Also fixes an early-exit bug the change surfaced: the existing
literal-filter optimization skipped tagging entirely, and returned
before any match was attempted, whenever the rule's required literal
was absent from the block. That is the right call for a normal rule,
but wrong for min, whose whole point is to detect too few matches,
including zero. Skipped now specifically when Min is set.

Real limitation, not fixed here: a sequence rule's declared scope is
always narrowed to sentences (see sentenceScope), so this counts
matches within one sentence, not across the several sentences of a
paragraph. Verified this precisely: built the actual vale binary and
ran a VerbTricolonDensity-shaped rule against real text, two
tricolons packed into one long sentence fire correctly with the
match count in the message, but two tricolons split across two
separate sentences of the same paragraph produce no alert, since
each sentence gets its own independent Run call with no state shared
between them. Counting across sentence boundaries would need that
scope-narrowing relaxed specifically for a rule that sets Max/Min,
plus a guard against a match spanning the sentence break to avoid a
new correctness problem. Neither is attempted here; documented on
the new fields and in the PR description rather than left implicit.

Refs vale-cli#1161
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant