Fix/dates 141 #143
Fix/dates 141 #143
Conversation
| @@ -154,7 +154,7 @@ const openrosa_xpath_extensions = function() { | |||
| 'decimal-date-time': function(r) { | |||
| if(arguments.length > 1) throw TOO_MANY_ARGS; | |||
|
|
|||
| const days = dateStringToDays(asString(r)); | |||
| const days = r.t === 'num' ? asNumber(r) : dateStringToDays(asString(r)); | |||
There was a problem hiding this comment.
While fixing 141 a bug in decimal-date-time surfaced. It did not convert number arguments into dateTimes.
|
|
||
| if (lhs.t === 'date') lhs = { t:'num', v:dateToDays(lhs.v) }; | ||
| if (rhs.t === 'date') rhs = { t:'num', v:dateToDays(rhs.v) }; | ||
|
|
There was a problem hiding this comment.
When converting dates to numbers if used as math operands or for comparisons, I believe we no longer need this complexity. Yay! 🥳
There was a problem hiding this comment.
What happens for logical or/and operators?
There was a problem hiding this comment.
I had a look and date strings '1969-12-30' or date('1969-12-30') are always true so I guess that's maybe bad.... 🤔 I cannot actually think of a use case where you would do date or date or date and date because it would be about an epoch check.
Things like this do work (and I'll add these test just in case):
["('2001-12-26' > '2001-12-25') or false()", true],
["false() or ('2001-12-26' > '2001-12-25')", true],
["('2001-12-26' < '2001-12-25') or false()", false],
["false() or ('2001-12-26' < '2001-12-25')", false],
["('2001-12-26' < '2001-12-25') and true()", false],
["true() and ('2001-12-26' < '2001-12-25')", false],
["('2001-12-26' > '2001-12-25') and true()", true],
["true() and ('2001-12-26' > '2001-12-25')", true],There was a problem hiding this comment.
Off the top of my head this makes sense - <date> or <date> would always be true, and the same with <date> and <date>... unless one or both were invalid dates (which there may not be support for).
I don't know if those examples add much, as they are really testing <date> greater-than <date> and <date>less-than <date>, and then <boolean> and <boolean>/<boolean> or <boolean>.
There was a problem hiding this comment.
Yes, indeed. Thank you. I didn't feel good about those tests actually. Am going to revert them! :)
|
|
Cannot request via GitHub, but if you happen to have some time, a review would be very welcome @alxndrsn! |
|
@MartijnR I think the date handling (edit: in OR) is a bit nuts, but within that context this all looks great 😁 |
This reverts commit 4e858ce.
Fixes enketo/enketo#866 (likely caused by a date misunderstanding on my part when guiding the development of this new evaluator - hence the incorrect test responses).
falseinstead oftrue(ODK Collect does it correctly).decimal-date-timewhen the argument is a number. The fix for 1 surfaced this issue with e.g. a complex expression likedecimal-date-time(today()- 60 )for which we had a test.A remaining difference with ODK Collect is that Enketo adds the local timezone offset. However, since it has been doing that since forever, it was considered outside the scope of this issue to investigate. I added comments to these tests though.