Skip to content

Implement ProperContains/ProperIn for both List and Interval#369

Open
dehall wants to merge 9 commits into
masterfrom
propercontains_properin
Open

Implement ProperContains/ProperIn for both List and Interval#369
dehall wants to merge 9 commits into
masterfrom
propercontains_properin

Conversation

@dehall

@dehall dehall commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

This PR implements the ProperContains and ProperIn operators, for Lists (implements #291) and Intervals (implements #292)
As of today, the spec reads as follows:

The ProperContains operator returns true if the first operand properly contains the second.

There are two overloads of this operator: List, T: The type of T must be the same as the element type of the list. Interval, T : The type of T must be the same as the point type of the interval.

For the List, T overload, this operator returns true if the given element is in the list, and it is not the only element in the list, using equality semantics, with the exception that null elements are considered equal. If the first argument is null, the result is false. If the second argument is null, the result is true if the list contains any null elements and at least one other element, and false otherwise.

For the Interval, T overload, this operator returns true if the given point is greater than the starting point of the interval, and less than the ending point of the interval, as determined by the Start and End operators. If precision is specified and the point type is a Date, DateTime, or Time type, comparisons used in the operation are performed at the specified precision. If the first argument is null, the result is false. If the second argument is null, the result is null.

(ProperIn is just the inverse - ProperContains is myList properly includes myItem, ProperIn is myItem properly included in myList)

However, there is an upcoming clarification to the spec that will clarify two aspects of these operators for Lists:

  1. that the list membership operators will [always] return true or false (i.e. we add "otherwise the result is false" to clarify)
  2. that the proper list operators (membership and otherwise) have list, not set, semantics.

This PR is implemented per these clarifications.

Note also that the cql tests are being updated as part of that clarification, see cqframework/cql-tests#119 , so I have copied the latest from that PR in here. As of now that PR has a couple approvals already but if it changes again I'll re-update.

Fortunately, the spec is pretty unambiguous for the Interval operators.

Pull requests into cql-execution require the following.
Submitter and reviewer should ✔ when done.
For items that are not-applicable, mark "N/A" and ✔.

Submitter:

  • This pull request describes why these changes were made
  • Code diff has been done and been reviewed (it does not contain: additional white space, not applicable code changes, debug statements, etc.)
  • Tests are included and test edge cases
  • Tests have been run locally and pass
  • Code coverage has not gone down and all code touched or added is covered.
  • Code passes lint and prettier (hint: use npm run test:plus to run tests, lint, and prettier)
  • [N/A] All dependent libraries are appropriately updated or have a corresponding PR related to this change
  • cql4browsers.js built with npm run build:browserify if source changed.

Reviewer:

Name:

  • Code is maintainable and reusable, reuses existing code and infrastructure where appropriate, and accomplishes the task’s purpose
  • The tests appropriately test the new code, including edge cases
  • You have tried to break the code

@codecov-commenter

codecov-commenter commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.37%. Comparing base (9877d3f) to head (cb5eb5c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
+ Coverage   88.30%   88.37%   +0.07%     
==========================================
  Files          54       54              
  Lines        4804     4836      +32     
  Branches     1350     1361      +11     
==========================================
+ Hits         4242     4274      +32     
  Misses        369      369              
  Partials      193      193              

☔ View full report in Codecov by Harness.
📢 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.

@dehall dehall requested a review from cmoesel June 30, 2026 20:37

@cmoesel cmoesel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good, @dehall. The implementation is straight-forward and the tests are robust. I've left a few small comments for your consideration. This also needs to be rebased (and it looks like there are conflicts).

Comment thread src/datatypes/interval.ts Outdated
Comment thread src/elm/overloaded.ts
Comment thread src/elm/overloaded.ts
Comment thread test/datatypes/interval-test.ts Outdated
Comment thread test/datatypes/interval-test.ts Outdated
Comment thread test/datatypes/interval-test.ts Outdated
Comment thread test/datatypes/interval-test.ts Outdated
Comment thread test/datatypes/interval-test.ts Outdated
Comment thread test/datatypes/interval-test.ts Outdated
Comment thread test/datatypes/interval-test.ts Outdated
@dehall dehall force-pushed the propercontains_properin branch from 773c9cc to cb5eb5c Compare July 1, 2026 16:26
@dehall

dehall commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @cmoesel , I think I've addressed all your comments, and rebased plus added LongInterval.properContains tests (cb5eb5c)

@dehall dehall requested a review from cmoesel July 1, 2026 16:33

@cmoesel cmoesel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, @dehall. This looks good and works well!

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.

3 participants