feat: change d2:minutesBetween arguments from LocalDate to Instant - #93
feat: change d2:minutesBetween arguments from LocalDate to Instant#93superskip wants to merge 6 commits into
LocalDate to Instant#93Conversation
|
|
| } | ||
|
|
||
| fun d2_minutesBetween(start: LocalDate?, end: LocalDate?): Int { | ||
| fun d2_minutesBetween(start: Instant?, end: Instant?): Double { |
There was a problem hiding this comment.
This change in the return type, is it safe? I think this function was only used by program indicator expressions, so that's ok. If it was used by program rules, we might have a sync issue if there is a parser version mismatch between client and server: for example, if a new Capture/Android app has a newer version of the expression-parser and the backend has an old one, the client could evaluate to 4.3 and the backend to 4, and it might produce an error at sync.
There was a problem hiding this comment.
Neither Tracker Capture or the Capture web app had implemented d2:minutesBetween. We did add the Int version to the parser in January, however: #92. So you are right, this might cause a headache to anyone who is using a server version released between January and August..
There was a problem hiding this comment.
This pull request also changes the implementation of d2:minutesBetween such that it no longer returns just multiples of 24 hours. I think this is a necessary change to make. It raises the same concern as with changing the return type, so we may take this opportunity to change both.
Not sure how we deal with the frontend / backend issue it will cause for certain backend versions though.
jbee
left a comment
jbee
left a comment
There was a problem hiding this comment.
AFAIK so far expression would only operate on dates and thus only LocalDate was required. This makes sense since the smallest data point interval we use in DHIS2 is "daily".
Any precision beyond days seems accidental to me unless you calculate some sort of statistic like an average which might yield a fraction number of days. But even in such a scenario you would not need Instant precision in the inputs as those would still be in steps of days. So I am doubtful this is actually required.
In addition I would think that adding another time related input type is quite a big change and would require to think about if not all time related parameters now must accept both types.
|
@jbee Thanks for your input! 🙏 According to the docs, there seems to be a concept of datetime in the world of program indicators. On the tracker side there is a data element of type datetime. At least for these data elements it would make sense to compute time difference in minutes. Adding a new "input type" like |
3 participants
toInstantTypeCoerciond2:minutesBetweenfrom Int to Double.The goal of this PR is to change the behaviour of
d2:minutesBetweenso that it can return values beyond the multiples of 24 hours.