android-fhir icon indicating copy to clipboard operation
android-fhir copied to clipboard

Add support for additional data during CQL evaluation

Open ndegwamartin opened this issue 1 year ago • 4 comments

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #2419

Description #2419

Alternative(s) considered Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Type Feature

Screenshots (if applicable)

Checklist

  • [x] I have read and acknowledged the Code of conduct.
  • [x] I have read the Contributing page.
  • [x] I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • [x] I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • [x] I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • [x] I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • [x] I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

ndegwamartin avatar Jan 31 '24 19:01 ndegwamartin

Please also run ./gradlew spotlessapply

MJ1998 avatar Feb 15 '24 10:02 MJ1998

Why not add this parameter additionalData to this API itself:-

fun evaluateLibrary(
    libraryUrl: String,
    patientId: String?,
    parameters: Parameters?,
    expressions: Set<String>?,
  ): IBaseParameters {

I'd figured this'd be a breaking change, but I suppose this is actually okay since we are still in alpha

ndegwamartin avatar Feb 15 '24 14:02 ndegwamartin

@MJ1998 additionally the the method is heavily overloaded in the same class so just added the new one. I've pushed an update that refactors it in line with the implementation of the other overloads.

ndegwamartin avatar Feb 15 '24 15:02 ndegwamartin

@ktarasenko @jingtang10 Can you take a look at this? Thanks

MJ1998 avatar Feb 19 '24 11:02 MJ1998

@vitorpamplona any idea why the Benchmarking tests would fail here?

ndegwamartin avatar Mar 20 '24 13:03 ndegwamartin

@vitorpamplona any idea why the Benchmarking tests would fail here?

nvm, figured out the issue

ndegwamartin avatar Mar 21 '24 09:03 ndegwamartin