multicontext values for cli tool#652
Conversation
|
Formatting check succeeded! |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #652 +/- ##
============================================
- Coverage 70.95% 70.95% -0.01%
- Complexity 153 154 +1
============================================
Files 473 473
Lines 22189 22193 +4
Branches 2898 2899 +1
============================================
+ Hits 15744 15746 +2
- Misses 4990 4992 +2
Partials 1455 1455 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| @ArgGroup(multiplicity = "0..1", exclusive = false) | ||
| @ArgGroup(exclusive = false) | ||
| public ContextParameter context; |
There was a problem hiding this comment.
Isn't the multiplicity change going to affect the cardinality of the context field? So this should be public List<ContextParameter> contexts?
There was a problem hiding this comment.
It currently reuses the singular context for each provided context-value. Wouldn't the cql file only have a single context?
There was a problem hiding this comment.
If that's how the command option parameters work, then okay, it just seems like if the multiplicity is on the "context" element, then that's the thing that would end up needing to be able to be list-valued? Maybe I just don't understand how the command option parameters are working.
There was a problem hiding this comment.
This should allow for behavior like this:
--context Patient --context-value id1 --context-value id2 --context-value id3
I confirmed this behavior with the unit test. You cannot add multiple contextName values
There was a problem hiding this comment.
we could add a second test where two contextNames are added which should cause a failure
There was a problem hiding this comment.
Actually, Bryn's right here. A given CQL library could have more than one context (some CQL in Encounter context, some in Patient) each of which would need to be set. We don't have a way to correlate across multiple contexts, so the name=value pairs are with respect to each Library evaluation.
There was a problem hiding this comment.
IOW, the CLI is currently set up to so that you parameterize one evaluation:
- the library
- the expressions
- the parameters
- the context parameters
It's not set up for multiple evaluations of a single library with different parameters.
I think this is a desirable behavior for this command. I think we need a new, different command with different semantics.
There was a problem hiding this comment.
Either that or reorganize the arguments so that you specify the library/expression independently of the rest of the parameters.
3 participants
enhancement to CLI tool to allow multiple context values to be specified for expression evaluation