Skip to content

Fix inclusive integer max regressions#402

Open
kendonB wants to merge 3 commits into
dictation-toolbox:feat/max-element-changesfrom
kendonB:review-pr-395
Open

Fix inclusive integer max regressions#402
kendonB wants to merge 3 commits into
dictation-toolbox:feat/max-element-changesfrom
kendonB:review-pr-395

Conversation

@kendonB
Copy link
Copy Markdown
Contributor

@kendonB kendonB commented Mar 21, 2026

This follow-up fixes the regressions introduced by making Integer-family max bounds inclusive in #395.
Summary:

  • keep Integer, IntegerRef, and ShortIntegerRef using inclusive public max bounds
  • preserve the caller-facing max value on Integer while still passing an exclusive upper bound to the internal builders
  • tighten Number series chunks back to 0..99 / 1..99 so the series form does not start accepting 100 after the inclusive change
  • tighten English calendar/time bounds that relied on the old exclusive behavior:
    • years: 2000..2099
    • absolute dates: 1..31
    • relative dates: 1..99 days
    • twelve-hour time: 1..12, minutes 0..59
    • military time: 00..23, minutes 0..59
  • add regression coverage for:
    • Integer(min=1, max=2) including the upper bound
    • Number rejecting three-digit series chunks
    • English calendar/time upper bounds
  • document the inclusive-range behavior in the language docs and changelog
    Validation:
  • text suite passes with the added calendar coverage

assisted by codex gpt5.4 xhigh

@kendonB
Copy link
Copy Markdown
Contributor Author

kendonB commented Mar 21, 2026

Added a follow-up commit to fix a Repetition.value() regression in the inclusive-max changes.

Problem:
When max == min + 1 (for example Repetition(child) or Repetition(child, min=1, max=2)), recognition succeeds for the maximum count, but get_repetitions() only walked the optional tail when self._max - self._min > 1. That drops the final repetition from the returned value.

Fix:

  • change the tail traversal condition in dragonfly/grammar/elements_basic.py from > 1 to > 0
  • add a focused text-engine regression test covering the two-item case

I pushed this as commit 9e2b5d9 on review-pr-395.
comment by 5.4 xhigh codex

Checked this one and it looks to be consistent with the idea of the original PR

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