Add multi-argument and single string distance variants #761
Conversation
it actually seems more intuitive to me that the (singular) argument to the existing XPath distance() function should be able to support either a node/nodeset (for example, containing a geotrace value), or a string with an equivalent literal value (or string function generating such). Many XPath functions behave this way... |
lognaturel
changed the title
|
Removing myself as a reviewer until we've merged getodk/xforms-spec#311 |
| bind("/data/point3").type("geopoint"), | ||
| bind("/data/point4").type("geopoint"), | ||
| bind("/data/point5").type("geopoint"), | ||
| bind("/data/distance").type("decimal").calculate("distance(/data/point1, '38.25021274773806 21.756382658677467 0 0', /data/point3, /data/point4, /data/point5)") |
There was a problem hiding this comment.
I'm guessing "point 2" is not pulled from a node to test that literals work fine here? I think if that's a concern it should move out to a separate test (or maybe a unit test of XPathFuncExpr) so it's not hidden in here.
There was a problem hiding this comment.
Allowing a mix of references and literals is an explicit requirement so I wanted to make sure it was tested. Maybe I split it out into a Scenario test with only references and one with a mix? We don't currently have unit tests that include type -- that ends up being more of an integration concern.
Closes #740
What has been done to verify that this works as intended?
Added a tests, tried sample form in Collect.
Why is this the best possible solution? Were any other approaches considered?
See getodk/xforms-spec#311 for spec consideration.
I went back and forth on where to include the logic around argument quantity and types. I decided to put it in
XPathFuncExprbecause that's most inline with what other functions do and it feels helpful to get an overview of variants at that top level.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Regression risks center around the existing distance function. It's possible the changes break that existing variant in some subtle way.
Do we need any specific form for testing your changes? If so, please attach one.
https://docs.google.com/spreadsheets/d/1Dv9lD33Bfqz4XqmTNMJXm4NK9Lw0TloKph1juU0Kins/edit#gid=1068911091
Does this change require updates to documentation? If so, please file an issue here and include the link below.
getodk/docs#1795