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.

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

Add support for additional data during CQL evaluation #2420

Merged
merged 20 commits into from
Mar 22, 2024

Conversation

ndegwamartin
Copy link
Collaborator

@ndegwamartin ndegwamartin commented Jan 31, 2024

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

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • 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 added a commit to opensrp/android-fhir that referenced this pull request Feb 7, 2024
* With unmerged PRS
- #9
- google#2420
- google#2178
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Feb 7, 2024
* With unmerged PRS
- #9
- google#2420
- google#2178
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Feb 9, 2024
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Feb 12, 2024
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Feb 12, 2024
@ndegwamartin ndegwamartin marked this pull request as ready for review February 14, 2024 08:20
- Ignore white spaces on XML diff assertion
Copy link
Collaborator

@MJ1998 MJ1998 left a comment

Choose a reason for hiding this comment

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

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

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

@MJ1998
Copy link
Collaborator

MJ1998 commented Feb 15, 2024

Please also run ./gradlew spotlessapply

@ndegwamartin
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@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.

@MJ1998
Copy link
Collaborator

MJ1998 commented Feb 19, 2024

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

Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

this seems like a legitimate change. but @brynrhodes can you please comment on the usage of additional data here? or @ndegwamartin can you please add some background on why this is necessary, what's the difference with prefetch data etc etc?

thanks!

Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

Thanks @ndegwamartin!

auto-merge was automatically disabled February 29, 2024 17:42

Head branch was pushed to by a user without write access

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Mar 19, 2024
… Knowledger

 - With unmerged PR #9
 - With unmerged PR google#2420
 - WUP PR google#2178
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Mar 19, 2024
… Knowledger

 - With unmerged PR #9
 - With unmerged PR google#2420
 - WUP PR google#2178
@ndegwamartin
Copy link
Collaborator Author

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

@ndegwamartin
Copy link
Collaborator Author

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

nvm, figured out the issue

@aditya-07 aditya-07 enabled auto-merge (squash) March 22, 2024 11:22
@aditya-07 aditya-07 merged commit a4f5f0f into google:master Mar 22, 2024
4 checks passed
@jingtang10 jingtang10 deleted the support-cql-eval-add-data branch March 25, 2024 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Add support for passing additional data to FHIROperator evaluateLibrary API
6 participants