Skip to content

Fix: properly between always behaved as plain between#1704

Merged
JPercival merged 2 commits intomainfrom
fix/properly-between-bug
Mar 16, 2026
Merged

Fix: properly between always behaved as plain between#1704
JPercival merged 2 commits intomainfrom
fix/properly-between-bug

Conversation

@JPercival
Copy link
Copy Markdown
Contributor

@JPercival JPercival commented Mar 14, 2026

Summary

  • visitBetweenExpression checked ctx.getChild(0).text == "properly" but child 0 is always the expression per the grammar rule expression 'properly'? 'between' expressionTerm 'and' expressionTerm. The keyword is at child index 1 when present, so isProper was always false.
  • properly between emitted And(GreaterOrEqual, LessOrEqual) instead of And(Greater, Less) — identical to plain between.
  • One-line fix: getChild(0) → getChild(1) in Cql2ElmVisitor.kt:1403.

Test plan

  • Added ProperlyBetweenTest regression test verifying:
    • between emits And(GreaterOrEqual, LessOrEqual)
    • properly between emits And(Greater, Less)
  • Test confirmed to fail without fix, pass with fix

🤖 Generated with Claude Code

@JPercival @claude
fix: properly between always behaved as plain between
3a456a7 
visitBetweenExpression checked ctx.getChild(0).text == "properly" but
child 0 is always the expression (per grammar rule
`expression 'properly'? 'between' expressionTerm 'and' expressionTerm`).
The keyword is at child index 1 when present. This meant isProper was
always false, so `properly between` emitted GreaterOrEqual/LessOrEqual
instead of Greater/Less.

Fix: change getChild(0) to getChild(1).

Add regression test ProperlyBetweenTest verifying:
- `between` emits And(GreaterOrEqual, LessOrEqual)
- `properly between` emits And(Greater, Less)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 14, 2026

Related Issues

The following open issues may be related to this PR:

Issue Title Score Matched Terms
#931 Library XML deserializer doesn't include annotations 9.5 instead, regression, com, code, generated, https, resources (path), java (path), src (path), main (path), org (path), cql (path), test (path)
#1505 'Collapse' on at least 2 closed datetime intervals without a high value always fails 8.5 com, https, per, always, without, between (path), cqframework (path), java (path), src (path), main (path), org (path), cql (path)
#1133 ToList is not implemented according to the spec 8.5 com, code, expression, https, line, cqframework (path), java (path), src (path), main (path), org (path), cql (path), test (path)
#1460 RetrieveEvaluator doesnt pass on valueset version to DataProvider/RetrieveProvider 8 pass, com, code, expression, https, cqframework (path), java (path), src (path), main (path), org (path), cql (path)
#1430 Correctly support CQL to FHIR (and vice versa) type mapping 8 instead, com, code, https, between (path), resources (path), cqframework (path), java (path), src (path), main (path), org (path), cql (path)

Tip: If this PR addresses any of these issues, please link them using Closes #NNN or Refs #NNN in the PR description.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 14, 2026

Formatting check succeeded!

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 14, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.97%. Comparing base (a21ac0b) to head (543cf45).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #1704   +/-   ##
=========================================
  Coverage     61.97%   61.97%           
  Complexity     3940     3940           
=========================================
  Files           210      210           
  Lines         20366    20366           
  Branches       3879     3879           
=========================================
  Hits          12621    12621           
  Misses         6138     6138           
  Partials       1607     1607

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Mar 14, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

c-schuler
c-schuler approved these changes Mar 15, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@c-schuler c-schuler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! The only suggestion I have is to include a test for the interval path, but not a blocker for approval.

Something like:
define TestIntervalProperlyBetween: Interval[1, 5] properly between Interval[0, 3] and Interval[4, 10]

@JPercival JPercival merged commit f33dbd0 into main Mar 16, 2026
9 of 10 checks passed
@JPercival JPercival deleted the fix/properly-between-bug branch March 16, 2026 13:56
JPercival added a commit that referenced this pull request Mar 16, 2026
@JPercival @claude
fix: properly between now correctly emits Greater/Less after main merge
77966da 
The properly-between fix (PR #1704) was merged to main, so the legacy
translator now correctly emits Greater/Less for `properly between`.
Update the new pipeline to match instead of replicating the old bug.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JPercival added a commit that referenced this pull request Mar 16, 2026
@JPercival @claude
docs: update LEGACY_ISSUES.md — all 3 bugs now fixed on main
e4dfe89 
- properly between: PR #1704 merged
- div/truncated divide: PR #1705 merged
- chunk-tracking error: PR #1706 merged

Renamed to "Translator Issues" since these aren't "legacy" yet.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JPercival added a commit that referenced this pull request Mar 28, 2026
@JPercival @claude
fix: properly between now correctly emits Greater/Less after main merge
39cdd06 
The properly-between fix (PR #1704) was merged to main, so the legacy
translator now correctly emits Greater/Less for `properly between`.
Update the new pipeline to match instead of replicating the old bug.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JPercival added a commit that referenced this pull request Mar 28, 2026
@JPercival @claude
docs: update LEGACY_ISSUES.md — all 3 bugs now fixed on main
68c7d1a 
- properly between: PR #1704 merged
- div/truncated divide: PR #1705 merged
- chunk-tracking error: PR #1706 merged

Renamed to "Translator Issues" since these aren't "legacy" yet.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@c-schuler c-schuler c-schuler approved these changes

@antvaset antvaset antvaset approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JPercival @c-schuler @antvaset