Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Explore elm types #267

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Explore elm types #267

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ea146e
explore types
csenn Mar 14, 2022
d14d548
a few more types
csenn Mar 14, 2022
cbee817
conditional types
csenn Mar 14, 2022
e7c3267
run prettier
csenn Mar 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
run prettier
  • Loading branch information
@csenn
csenn committed Mar 14, 2022
commit e7c3267eabba2a688617de3d735ba444b8f8478e
24 changes: 12 additions & 12 deletions src/datatypes/quantity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,10 @@ export class Quantity {
return this.value <= otherVal;
}
}
return null
return null;
}

sameOrAfter(other: any): null | boolean {
sameOrAfter(other: any): null | boolean {
if (other != null && other.isQuantity) {
const otherVal = convertUnit(other.value, other.unit, this.unit);
if (otherVal == null) {
Expand All @@ -58,10 +58,10 @@ export class Quantity {
return this.value >= otherVal;
}
}
return null
return null;
}

after(other: any): null | boolean {
after(other: any): null | boolean {
if (other != null && other.isQuantity) {
const otherVal = convertUnit(other.value, other.unit, this.unit);
if (otherVal == null) {
Expand All @@ -70,10 +70,10 @@ export class Quantity {
return this.value > otherVal;
}
}
return null
return null;
}

before(other: any): null | boolean {
before(other: any): null | boolean {
if (other != null && other.isQuantity) {
const otherVal = convertUnit(other.value, other.unit, this.unit);
if (otherVal == null) {
Expand All @@ -82,10 +82,10 @@ export class Quantity {
return this.value < otherVal;
}
}
return null
return null;
}

equals(other: any): null | boolean {
equals(other: any): null | boolean {
if (other != null && other.isQuantity) {
if ((!this.unit && other.unit) || (this.unit && !other.unit)) {
return false;
Expand All @@ -100,7 +100,7 @@ export class Quantity {
}
}
}
return null
return null;
}

convertUnit(toUnit: any): Quantity {
Expand Down Expand Up @@ -215,14 +215,14 @@ export function doDivision(a: Quantity | null, b: Quantity | null): Quantity | n
if (a != null && a.isQuantity) {
return a.dividedBy(b);
}
return null
return null;
}

export function doMultiplication(a: Quantity | null, b: Quantity | null): Quantity | null {
if (a != null && a.isQuantity) {
return a.multiplyBy(b);
} else if (b!= null && b.isQuantity) {
} else if (b != null && b.isQuantity) {
return b.multiplyBy(a);
}
return null
return null;
}
12 changes: 6 additions & 6 deletions src/elm/arithmetic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { Uncertainty } from '../datatypes/uncertainty';
import { Context } from '../runtime/context';
import { build } from './builder';
import { DateTime } from '../datatypes/datetime';
import ELM from '../types/elm'
import ELM from '../types/elm';

export class Add extends Expression {
constructor(json: ELM.Add) {
Expand Down Expand Up @@ -390,14 +390,14 @@ export class MinValue extends Expression {
'{urn:hl7-org:elm-types:r1}Time': MathUtil.MIN_TIME_VALUE
};

valueType: keyof typeof MinValue.MIN_VALUES
valueType: keyof typeof MinValue.MIN_VALUES;

constructor(json: ELM.MinValue) {
super(json);
if (json.valueType in MinValue.MIN_VALUES) {
this.valueType = json.valueType as keyof typeof MinValue.MIN_VALUES
this.valueType = json.valueType as keyof typeof MinValue.MIN_VALUES;
Copy link
Contributor

Choose a reason for hiding this comment

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

Small (opinionated) comment. Now that we use keyof typeof MinValue.MIN_VALUES in more than one place, maybe we can bring it out to its own type outside of the class? E.g.:

type MinValueType = keyof typeof MinValue.MIN_VALUES;

Then we can just re-use this type where applicable. Same applies to max values below

Copy link
Author

Choose a reason for hiding this comment

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

It also does make sense to pull MIN_VALUE out, and maybe even use an ENUM instead of a dict. But it was a little weird to even have to use as keyof typeof MinValue.MIN_VALUES... I thought Typescript would be able to infer that since json.valueType in MinValue.MIN_VALUES must be true

} else {
throw new Error(`Not expecting MIN_VALUE: ${json.valueType})`)
throw new Error(`Not expecting MIN_VALUE: ${json.valueType})`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This error check breaks the arithmetic tests, but for a very logical reason lol. There is a test case that asserts that any incorrect MinValue provided should throw an error during execution. The generated ELM for this test case is here.

Since the constructor now throws an error if provided a valueType that we don't recognize.

I'm going to defer to @cmoesel here on how exactly to proceed. I think I might lean towards not throwing an error in a constructor, but I see the merit in the code that you've added.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah def agree with you, both ways have merit. Here is a StackOverflow discussing. I semi-lean towards validation in the constructor as long as that runs over the whole ELM tree as soon as the ELM wrapper is instantiated, just so that it doesn't become possible for execution to 'skip' certain parts of the ELM tree depending on the input patient data (only exec MinValue node if Patient has Condition Y) which could then allow bugs to escape tests more easily. Not sure if that's applicable though?

Either way, MIN_VALUES could be an ENUM but SHOULD NOT be added to the ELM generated from the XML because the mappings to MathUtil.MIN_INT_VALUE seem to be numbers that are only specific to the JS implementation.

Copy link
Member

Choose a reason for hiding this comment

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

I can see both arguments -- including the desire to fail immediately upon loading the ELM (rather than on execution, just in case execution never occurs). That said the spec does say this:

For any other type, attempting to invoke minimum results in an error.

One could argue that "attempting to invoke" implies an error on execution, not on loading. (As far as I can tell, this is how the Java cql-engine implementation behaves as well).

}
}

Expand Down Expand Up @@ -430,9 +430,9 @@ export class MaxValue extends Expression {
constructor(json: ELM.MaxValue) {
super(json);
if (json.valueType in MaxValue.MAX_VALUES) {
this.valueType = json.valueType as keyof typeof MaxValue.MAX_VALUES
this.valueType = json.valueType as keyof typeof MaxValue.MAX_VALUES;
} else {
throw new Error(`Not expecting Max_VALUE: ${json.valueType})`)
throw new Error(`Not expecting Max_VALUE: ${json.valueType})`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same deal here as my comment about errors for MinValue.

}
}

Expand Down
18 changes: 9 additions & 9 deletions src/elm/clinical.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Expression } from './expression';
import * as dt from '../datatypes/datatypes';
import { Context } from '../runtime/context';
import { build } from './builder';
import ELM from '../types/elm'
import ELM from '../types/elm';

export class ValueSetDef extends Expression {
name: string;
Expand Down Expand Up @@ -54,7 +54,7 @@ export class AnyInValueSet extends Expression {
super(json);
this.codes = build(json.codes);
if (json.valueset?.type !== 'ValueSetRef') {
throw new Error('Expecting ValueSetRef Expression')
throw new Error('Expecting ValueSetRef Expression');
}
this.valueset = new ValueSetRef(json.valueset);
}
Expand All @@ -79,7 +79,7 @@ export class InValueSet extends Expression {
super(json);
this.code = build(json.code);
if (json.valueset?.type !== 'ValueSetRef') {
throw new Error('Expecting ValueSetRef Expression')
throw new Error('Expecting ValueSetRef Expression');
}
this.valueset = new ValueSetRef(json.valueset);
}
Expand Down Expand Up @@ -196,7 +196,7 @@ export class ConceptDef extends Expression {
display?: string;

constructor(json: ELM.ConceptDef) {
// @ts-ignore
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as initial comment with more explanation of ts-ignore and disabling the lint rule

super(json);
this.name = json.name;
this.display = json.display;
Expand All @@ -215,7 +215,7 @@ export class ConceptDef extends Expression {
export class ConceptRef extends Expression {
name: string;

constructor(json: ELM.ConceptRef) {
constructor(json: ELM.ConceptRef) {
super(json);
this.name = json.name || '';
Copy link
Contributor

Choose a reason for hiding this comment

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

ugh you know the drill at this point. My fault again, property can be optional. It's almost like I should've read the dang manual when doing this conversion. Sorry

Copy link
Author

@csenn csenn Mar 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I personally don't hate defaulting to empty strings because usually you just do a check like

if (json.name) {
   // Do Something
}

and null or empty string behave the same way. It also makes the code and types simpler sometimes without having to always check for null in cases where you want to do something like

json.name.toLowerCase()

But the broader discussion goes back to whether there are cases where we'd actually want to update the XML spec itself and change certain properties to required or not. There are pros and cons to having the single source of truth for the ELM spec. But it could get messy if we were to say manually start editing the generated ELM Typescript Types, because then we would not be able to generate it again later when the XML schema changes.

Copy link
Member

Choose a reason for hiding this comment

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

At this point, unless there is a strong reason for it, I'd shy away from introducing changes to the ELM specification. Those changes are expensive given the governance process on CQL. In cases where something isn't clearly wrong, it may be better just to accommodate for it.

}
Expand Down Expand Up @@ -259,7 +259,7 @@ export class CalculateAge extends Expression {
constructor(json: ELM.CalculateAge) {
super(json);
if (!json.precision) {
throw new Error('Precision is required')
throw new Error('Precision is required');
}
this.precision = json.precision;
}
Expand All @@ -278,12 +278,12 @@ export class CalculateAge extends Expression {
}

export class CalculateAgeAt extends Expression {
precision: ELM.DateTimePrecision;
precision: ELM.DateTimePrecision;

constructor(json: ELM.CalculateAgeAt) {
super(json);
if (!json.precision) {
throw new Error('Precision is required')
throw new Error('Precision is required');
}
this.precision = json.precision;
}
Expand Down
4 changes: 2 additions & 2 deletions src/elm/expression.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ import { Context } from '../runtime/context';
import { typeIsArray } from '../util/util';

import { build } from './builder';
import ELM from '../types/elm'
import ELM from '../types/elm';

export class Expression {
localId?: string;
arg?: any;
args?: any[];

constructor(json: ELM.Expression) {
if ('operand' in json){
if ('operand' in json) {
if (json.operand != null) {
const op = build(json.operand);
if (typeIsArray(json.operand)) {
Expand Down
2 changes: 1 addition & 1 deletion src/elm/literal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Context } from '../runtime/context';
import { Expression } from './expression';
import ELM from '../types/elm'
import ELM from '../types/elm';

export class Literal extends Expression {
valueType: string;
Expand Down
Loading