Vs firstclass #349
Vs firstclass #349
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #349 +/- ##
==========================================
+ Coverage 87.47% 87.55% +0.08%
==========================================
Files 52 52
Lines 4566 4596 +30
Branches 1291 1295 +4
==========================================
+ Hits 3994 4024 +30
- Misses 358 359 +1
+ Partials 214 213 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- 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
Create internal vs class Export correct types and delay expansion of vs
hossenlopp
left a comment
hossenlopp
left a comment
There was a problem hiding this comment.
First pass review. I mainly looked from the aspect of the change in code coverage. Found a few things worth changing and others worth discussing.
elsaperelli
left a comment
elsaperelli
left a comment
There was a problem hiding this comment.
Just a few comments/questions for now but looks good!
hossenlopp
left a comment
hossenlopp
left a comment
There was a problem hiding this comment.
Got a few more suggestions on tests.
Co-authored-by: hossenlopp <hossenlopp@mitre.org>
cmoesel
left a comment
cmoesel
left a comment
There was a problem hiding this comment.
Fantastic work, @lmd59 -- and great reviews, @hossenlopp and @elsaperelli!
I left one tiny comment about a test that I think could be improved, but I wouldn't let it hold up this PR from being merged. So... if @lmd59 has time to address it, great! Let's do that! But if she doesn't has time, then LGTM!
test/elm/clinical/clinical-test.ts
Outdated
| const vs = await this['unknown Three Arg'].exec(this.ctx); | ||
| vs.id.should.equal('1.2.3.4.5.6.7.8.9'); | ||
| vs.version.should.equal('1'); | ||
| vs.codesystems.length.should.equal(1); |
There was a problem hiding this comment.
Ideally we should check that the codesystem we get is the one we expect (instead of just checking that the codesystems array is length 1).
There was a problem hiding this comment.
Updated this test and the one below it in 65cc22f.
Addresses #226 by creating a new type (
CQLValueSet) that aligns with the CQL 1.5 specification defined ValueSet type.Note: This type naming is currently
CQLValueSetso as not to change the interface ofValueSet, but this is intended to be a temporary decision that should be updated for the next major update. At the next major update, changeCQLValueSet->ValueSetandValueSet->ValueSetExpansion.This PR shifts valueset resolution to the point that it is required, such as when
hasMatchorexpandis called. It does not cache valueset expansions in the context and leaves valueset caching up to the code service. This includes an update to equivalence that only expands valuesets to check matching codes if the valuesets do not have an exact match of id/version. The PR also adds a consistent error if passed valuesets are not able to be resolved by the code service.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:
npm run test:plusto run tests, lint, and prettier)cql4browsers.jsbuilt withnpm run build:browserifyif source changed.~ Code coverage has not gone down, but not all added/touched code is covered as code lines represent unlikely cases that would be difficult to test.
Reviewer:
Name: