-
Notifications
You must be signed in to change notification settings - Fork 30
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.
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
Explore elm types #267
base: master
Are you sure you want to change the base?
Explore elm types #267
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,9 +11,10 @@ import { Uncertainty } from '../datatypes/uncertainty'; | |
import { Context } from '../runtime/context'; | ||
import { build } from './builder'; | ||
import { DateTime } from '../datatypes/datetime'; | ||
import ELM from '../types/elm'; | ||
|
||
export class Add extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Add) { | ||
super(json); | ||
} | ||
|
||
|
@@ -56,7 +57,7 @@ export class Add extends Expression { | |
} | ||
|
||
export class Subtract extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Subtract) { | ||
super(json); | ||
} | ||
|
||
|
@@ -94,7 +95,7 @@ export class Subtract extends Expression { | |
} | ||
|
||
export class Multiply extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Multiply) { | ||
super(json); | ||
} | ||
|
||
|
@@ -132,7 +133,7 @@ export class Multiply extends Expression { | |
} | ||
|
||
export class Divide extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Divide) { | ||
super(json); | ||
} | ||
|
||
|
@@ -172,7 +173,7 @@ export class Divide extends Expression { | |
} | ||
|
||
export class TruncatedDivide extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.TruncatedDivide) { | ||
super(json); | ||
} | ||
|
||
|
@@ -193,7 +194,7 @@ export class TruncatedDivide extends Expression { | |
} | ||
|
||
export class Modulo extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Modulo) { | ||
super(json); | ||
} | ||
|
||
|
@@ -210,7 +211,7 @@ export class Modulo extends Expression { | |
} | ||
|
||
export class Ceiling extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Ceiling) { | ||
super(json); | ||
} | ||
|
||
|
@@ -225,7 +226,7 @@ export class Ceiling extends Expression { | |
} | ||
|
||
export class Floor extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Floor) { | ||
super(json); | ||
} | ||
|
||
|
@@ -240,7 +241,7 @@ export class Floor extends Expression { | |
} | ||
|
||
export class Truncate extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Truncate) { | ||
super(json); | ||
} | ||
|
||
|
@@ -254,7 +255,7 @@ export class Truncate extends Expression { | |
} | ||
} | ||
export class Abs extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Abs) { | ||
super(json); | ||
} | ||
|
||
|
@@ -271,7 +272,7 @@ export class Abs extends Expression { | |
} | ||
|
||
export class Negate extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Negate) { | ||
super(json); | ||
} | ||
|
||
|
@@ -290,7 +291,7 @@ export class Negate extends Expression { | |
export class Round extends Expression { | ||
precision: any; | ||
|
||
constructor(json: any) { | ||
constructor(json: ELM.Round) { | ||
super(json); | ||
this.precision = build(json.precision); | ||
} | ||
|
@@ -307,7 +308,7 @@ export class Round extends Expression { | |
} | ||
|
||
export class Ln extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Ln) { | ||
super(json); | ||
} | ||
|
||
|
@@ -324,7 +325,7 @@ export class Ln extends Expression { | |
} | ||
|
||
export class Exp extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Exp) { | ||
super(json); | ||
} | ||
|
||
|
@@ -344,7 +345,7 @@ export class Exp extends Expression { | |
} | ||
|
||
export class Log extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Log) { | ||
super(json); | ||
} | ||
|
||
|
@@ -361,7 +362,7 @@ export class Log extends Expression { | |
} | ||
|
||
export class Power extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Power) { | ||
super(json); | ||
} | ||
|
||
|
@@ -381,7 +382,7 @@ export class Power extends Expression { | |
} | ||
|
||
export class MinValue extends Expression { | ||
static readonly MIN_VALUES = { | ||
static MIN_VALUES = { | ||
'{urn:hl7-org:elm-types:r1}Integer': MathUtil.MIN_INT_VALUE, | ||
'{urn:hl7-org:elm-types:r1}Decimal': MathUtil.MIN_FLOAT_VALUE, | ||
'{urn:hl7-org:elm-types:r1}DateTime': MathUtil.MIN_DATETIME_VALUE, | ||
|
@@ -391,9 +392,13 @@ export class MinValue extends Expression { | |
|
||
valueType: keyof typeof MinValue.MIN_VALUES; | ||
|
||
constructor(json: any) { | ||
constructor(json: ELM.MinValue) { | ||
super(json); | ||
this.valueType = json.valueType; | ||
if (json.valueType in MinValue.MIN_VALUES) { | ||
this.valueType = json.valueType as keyof typeof MinValue.MIN_VALUES; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small (opinionated) comment. Now that we use type MinValueType = keyof typeof MinValue.MIN_VALUES; Then we can just re-use this type where applicable. Same applies to max values below There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} else { | ||
throw new Error(`Not expecting MIN_VALUE: ${json.valueType})`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Since the constructor now throws an error if provided a 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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). |
||
} | ||
} | ||
|
||
exec(ctx: Context) { | ||
|
@@ -422,9 +427,13 @@ export class MaxValue extends Expression { | |
|
||
valueType: keyof typeof MaxValue.MAX_VALUES; | ||
|
||
constructor(json: any) { | ||
constructor(json: ELM.MaxValue) { | ||
super(json); | ||
this.valueType = json.valueType; | ||
if (json.valueType in MaxValue.MAX_VALUES) { | ||
this.valueType = json.valueType as keyof typeof MaxValue.MAX_VALUES; | ||
} else { | ||
throw new Error(`Not expecting Max_VALUE: ${json.valueType})`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same deal here as my comment about errors for MinValue. |
||
} | ||
} | ||
|
||
exec(ctx: Context) { | ||
|
@@ -443,7 +452,7 @@ export class MaxValue extends Expression { | |
} | ||
|
||
export class Successor extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Successor) { | ||
super(json); | ||
} | ||
|
||
|
@@ -472,7 +481,7 @@ export class Successor extends Expression { | |
} | ||
|
||
export class Predecessor extends Expression { | ||
constructor(json: any) { | ||
constructor(json: ELM.Predecessor) { | ||
super(json); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to take out the
readonly
qualifier here? The value ofMIN_VALUES
never gets re-assigned. I saw thatMAX_VALUES
below still hasreadonly
, so figured I'd ask about the changeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, just added readonly back!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@csenn - I'm taking a look at this PR again, as it would be great to get it resolved. I noticed that you said you added the
readonly
back on line 385, but when I look at the current code on this branch, I don't see it. Perhaps you have not pushed it back up?