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

Rename install API to index for file in knowledge manager #2634

Merged
merged 19 commits into from
Aug 2, 2024

Conversation

jingtang10
Copy link
Collaborator

@jingtang10 jingtang10 commented Jul 22, 2024

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

Description
Rename install API to index for file in knowledge manager

Note In this PR we also change the test dependency in the workflow module to the current version of knowledge manager instead of the published version. This makes sure we're testing against the latest Knowledge Manager APIs.

Alternative(s) considered
NA

Type
Code health

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

@jingtang10 for you consideration, please check out my comment on the Github ticket here. If the recommendations there make sense then this might be an appropriate PR to piggy back the enhancement on.

Copy link
Collaborator

@FikriMilano FikriMilano left a comment

Choose a reason for hiding this comment

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

The change to rename the API looks good to merge.

However, we need to clarify and also document the reasoning whether the KnowledgeManager Library should be able to handle non MetadataResource (such as Patient, Practitioner) or not.

@jingtang10
Copy link
Collaborator Author

@jingtang10 for you consideration, please check out my comment on the Github ticket here. If the recommendations there make sense then this might be an appropriate PR to piggy back the enhancement on.

thanks martin - that will be handled in a subsequent pr - let's keep the scope of this pr smaller.

@jingtang10 jingtang10 enabled auto-merge (squash) July 31, 2024 16:52
Copy link
Contributor

@aditya-07 aditya-07 left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@jingtang10 jingtang10 merged commit 603792f into google:master Aug 2, 2024
6 checks passed
@jingtang10 jingtang10 deleted the knowledge branch August 2, 2024 12:11
ndegwamartin pushed a commit to opensrp/android-fhir that referenced this pull request Sep 10, 2024
* Rename install to index for file

* Revert workflow unit tests since they use published knowledge manager

* Remove extra dependency in workflow build file

* Address review comments

* Fix smart immunizations test

* Update knowledge/src/main/java/com/google/android/fhir/knowledge/KnowledgeManager.kt

Co-authored-by: aditya-07 <[email protected]>

* Update knowledge/src/main/java/com/google/android/fhir/knowledge/KnowledgeManager.kt

Co-authored-by: aditya-07 <[email protected]>

* Run spotless

---------

Co-authored-by: aditya-07 <[email protected]>
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.

4 participants