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

Implement calculated-expression extension

Open maimoonak opened this issue 2 years ago • 13 comments

Fixes #971 #1173

Description Implemented the calculated expression extension. The feature also calculated cyclic dependency, and prevents the infinite calls of callback

Type Choose one: Feature

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

maimoonak avatar May 11 '22 06:05 maimoonak

@RaaziaTarique kindly take a 1st pass review

f-odhiambo avatar May 12 '22 11:05 f-odhiambo

Codecov Report

Merging #1380 (6c2e0e4) into master (da1b5b3) will increase coverage by 32.61%. The diff coverage is 14.07%.

:exclamation: Current head 6c2e0e4 differs from pull request most recent head 86e7a2b. Consider uploading reports for the commit 86e7a2b to get more accurate results

@@              Coverage Diff              @@
##             master    #1380       +/-   ##
=============================================
+ Coverage          0   32.61%   +32.61%     
- Complexity        0      320      +320     
=============================================
  Files             0      151      +151     
  Lines             0     5273     +5273     
  Branches          0      942      +942     
=============================================
+ Hits              0     1720     +1720     
- Misses            0     3307     +3307     
- Partials          0      246      +246     
Impacted Files Coverage Δ
...hir/datacapture/MoreQuestionnaireItemComponents.kt 14.28% <0.00%> (ø)
...re/MoreQuestionnaireResponseItemAnswerComponent.kt 0.00% <0.00%> (ø)
...android/fhir/datacapture/QuestionnaireViewModel.kt 0.00% <0.00%> (ø)
...d/fhir/datacapture/fhirpath/ExpressionEvaluator.kt 0.00% <0.00%> (ø)
...android/fhir/datacapture/mapping/ResourceMapper.kt 0.00% <0.00%> (ø)
...roid/fhir/datacapture/validation/ValidationUtil.kt 33.33% <0.00%> (ø)
...ws/QuestionnaireItemDatePickerViewHolderFactory.kt 62.90% <50.00%> (ø)
...stionnaireItemEditTextQuantityViewHolderFactory.kt 73.07% <86.66%> (ø)
...droid/fhir/db/impl/entities/DateTimeIndexEntity.kt 100.00% <0.00%> (ø)
...fhir/search/filter/QuantityParamFilterCriterion.kt 100.00% <0.00%> (ø)
... and 149 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar May 12 '22 16:05 codecov[bot]

Assigning to @aditya-07 for review.

Tarun-Bhardwaj avatar May 16 '22 10:05 Tarun-Bhardwaj

@jingtang10 @aditya-07 Please review the changes

maimoonak avatar May 31 '22 21:05 maimoonak

@aditya-07 @jingtang10 could you please review the changes?

Tarun-Bhardwaj avatar Jun 15 '22 11:06 Tarun-Bhardwaj

@joiskash can you please leave a review? thanks!

jingtang10 avatar Jul 01 '22 16:07 jingtang10

Can you attach a screenshot please?

@shelaghm not sure if we want to show case this in the layout screen of the catalog app... i kind feel like this is better off in the components page... that is if we want to showcase this at all.

I agree that it's likely more useful to show in components page, not layout page. Having a screenshot that I can look at of this components will he helpful for me to provide more specific guidance.

shelaghm avatar Jul 01 '22 16:07 shelaghm

@maimoonak could you please advise an ETA by when these changes can be ready for review?

Tarun-Bhardwaj avatar Jul 20 '22 14:07 Tarun-Bhardwaj

@Tarun-Bhardwaj I have resolved all conflicts with main, synced the code with new changes as well just now. The issue https://github.com/google/android-fhir/issues/1498 needs to be resolved to run the catalog (without extra needless edit-text). But this PR and functionality does not depend on this issue and is working as expected. (Below is a working example)

ezgif com-gif-maker

maimoonak avatar Jul 20 '22 14:07 maimoonak

@maimoonak , could you please advise the status of these changes? Is there an ETA by when we can merge this PR?

Tarun-Bhardwaj avatar Aug 23 '22 05:08 Tarun-Bhardwaj

@Tarun-Bhardwaj @jingtang10 kindly review the PR.

maimoonak avatar Sep 05 '22 06:09 maimoonak

can you please make sure to reply to the review comments and mark the conversations as resolved if the comment is addressed in the code. once that's done please reassign back to me. thanks

jingtang10 avatar Sep 15 '22 12:09 jingtang10

@maimoonak please reassign to me once all comments are addressed

jingtang10 avatar Sep 22 '22 10:09 jingtang10

@jingtang10 I have found few libraries for graph generation. I am still exploring these and if its possible to use any for our expression dependency graph https://github.com/systek/dataflow https://github.com/jgrapht/jgrapht/ https://github.com/google/guava/wiki/GraphsExplained https://github.com/nidi3/graphviz-java#user-content-api

maimoonak avatar Oct 10 '22 17:10 maimoonak

@jingtang10 I have found few libraries for graph generation. I am still exploring these and if its possible to use any for our expression dependency graph https://github.com/systek/dataflow https://github.com/jgrapht/jgrapht/ https://github.com/google/guava/wiki/GraphsExplained https://github.com/nidi3/graphviz-java#user-content-api

Thanks Maimoona - what I had in mind was just to use a basic data structure to represent the dependencies. I wasn't sure that we'd need to use any external libraries for this.

We already have some auxiliary data strcuture such as the parent map in the view model - this would be similar to that I thought.

jingtang10 avatar Oct 11 '22 09:10 jingtang10

@maimoonak i turned on auto-merge. so if you update the branch it should merge automatically.

thanks

jingtang10 avatar Oct 11 '22 17:10 jingtang10