Skip to content

Added CodeSystemRef and updated CodeSystem type - #346

Merged
hossenlopp merged 8 commits into
masterfrom
update-codesystem
Aug 11, 2025
Merged

Added CodeSystemRef and updated CodeSystem type#346
hossenlopp merged 8 commits into
masterfrom
update-codesystem

Conversation

@elsaperelli

Copy link
Copy Markdown
Contributor

Summary

This PR addresses #259 to add capability for CodeSystemRef. It also adds name to the CodeSystem type, something that was introduced in CQL 1.5.

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)
  • [✔] 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 Jul 29, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.46%. Comparing base (2a3dfb2) to head (4395de4).

Files with missing lines Patch % Lines
src/elm/clinical.ts 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #346      +/-   ##
==========================================
- Coverage   87.47%   87.46%   -0.01%     
==========================================
  Files          52       52              
  Lines        4550     4555       +5     
  Branches     1284     1285       +1     
==========================================
+ Hits         3980     3984       +4     
  Misses        361      361              
- Partials      209      210       +1     

☔ 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.

@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 is looking pretty good. I've left a few comments:

  • one about the CodeSystem constructor and backwards-compatibility
  • and a couple about one of the tests

Looking for feedback on the constructor and backwards compatibility.

Comment thread src/datatypes/clinical.ts Outdated
Comment thread test/datatypes/clinical-test.ts Outdated
Comment thread test/datatypes/clinical-test.ts
@elsaperelli
elsaperelli requested a review from cmoesel July 31, 2025 17:15

@hossenlopp hossenlopp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CodeSystemRef should be able to reference a CodeSystem in another library. If CQL-to-ELM can't translate that right then maybe either not implement it or just not test it.

Comment thread src/elm/clinical.ts
}
}

export class CodeSystemRef extends Expression {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ELM specification says this could reference CodeSystems in other libraries. The optional libraryName should be pulled out of the json and used when getting the code system from the context. Which will require changes to the Context (and Library) getCodeSystem function to support the library parameter. ValueSetRef and getValueSet can be used as a good reference for this.

http://cql.hl7.org/04-logicalspecification.html#codesystemref

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.

Ooooh. Good call, @hossenlopp. I just tested this and confirmed that the translator does support referencing CodeSystems from another library.

Comment thread test/datatypes/clinical-test.ts Outdated
cmoesel added 2 commits August 3, 2025 15:54
- Add libraryName to CodeSystemRef
- Update Library to resolve CodeSystems in other libraries
- Fix bug that put included library codesystems into the main library
- Fix bug that returned wrong value set when main library and included library had value sets with same name
- Add tests for resolving value sets from included libraries
@cmoesel
cmoesel force-pushed the update-codesystem branch from 3bcff99 to 7d967de Compare August 3, 2025 20:25
@cmoesel

cmoesel commented Aug 4, 2025

Copy link
Copy Markdown
Member

I've updated this branch to add support for code systems from other libraries. Along the way I noticed a couple of related things that needed to be fixed:

  • CodeSystems from included libraries were being put into the main library codesystems collection -- and overwriting the ones from the main library in cases where the name was the same! Whoops!
  • When resolving ValueSetRefs, the logic was only looking at the specified library name if it couldn't find a ValueSet with the requested name in the main library. Which means that if you had a VS in the main library and a VS in an included library with the same name, it always gave you the former, even if you asked for the latter! Whoops (again)!

@elsaperelli

Copy link
Copy Markdown
Contributor Author

Reviewed @cmoesel's updates and they all look good to me! Approved!

@hossenlopp hossenlopp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Additional changes and tests look good.

@hossenlopp
hossenlopp merged commit cdd354d into master Aug 11, 2025
8 checks passed
@hossenlopp
hossenlopp deleted the update-codesystem branch August 11, 2025 15:31
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.

4 participants