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

Support definition based extraction for lists with Non - primitive types #1788

Merged

Conversation

vbothe23
Copy link
Contributor

@vbothe23 vbothe23 commented Jan 3, 2023

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

Fixes #1769

Description
Issue:
In addAnswerToListField if the answerValue is of non-primitive type then the primitiveValue will be null and app crashes with NullPointerException.

Fix:
Added isPrimitiveType check to handle answerValue of primitive and non-primitive types when extracting to list in definition based extraction.

Alternative(s) considered
No

Type
Choose one: Bug fix

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

Copy link
Collaborator

@joiskash joiskash left a comment

Choose a reason for hiding this comment

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

Looks good! Congratulations on your first PR, I left a small comment please have a look at it

@vbothe23 vbothe23 removed their assignment Jan 4, 2023
@omarismail94 omarismail94 self-assigned this Jan 4, 2023
Copy link
Contributor

@omarismail94 omarismail94 left a comment

Choose a reason for hiding this comment

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

Nice change @vbothe23 ! Left a few comments

@omarismail94 omarismail94 assigned vbothe23 and unassigned omarismail94 Jan 4, 2023
@vbothe23 vbothe23 requested a review from joiskash January 6, 2023 07:48
Copy link
Collaborator

@joiskash joiskash left a comment

Choose a reason for hiding this comment

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

LGTM!

@joiskash joiskash assigned omarismail94 and unassigned vbothe23 Jan 6, 2023
@omarismail94
Copy link
Contributor

LGTM, and will let @jingtang10 give the final approval!

Jing - I reviewed the PR and the suggestion I made was to use a method, fhirType(), that works on both primitives and non-primitives

@omarismail94 omarismail94 enabled auto-merge (squash) January 16, 2023 10:42
@omarismail94 omarismail94 merged commit da528d4 into google:master Jan 16, 2023
@jingtang10
Copy link
Collaborator

@joiskash @omarismail94 @vbothe23 can you please help addressing my above comments in a follow up cl?

@joiskash
Copy link
Collaborator

Sure @vbothe23 will do this ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Definition Based Extraction: Issue in extracting Observation value of type valueCoding
5 participants