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

Add multi-argument and single string distance variants #761

Merged
merged 3 commits into from
May 21, 2024

Conversation

lognaturel
Copy link
Member

@lognaturel lognaturel commented May 10, 2024

Closes #740
Adds multiple arguments as described at getodk/xforms-spec#311

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 XPathFuncExpr because 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

@tiritea
Copy link
Contributor

tiritea commented May 14, 2024

An alternative would be to always only allow a single arg but to allow that arg to be a literal string rather than only a nodeset.

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 lognaturel changed the title Add multi-argument distance variant Add multi-argument and single string distance variants May 14, 2024
@lognaturel lognaturel requested a review from seadowg May 14, 2024 04:22
@seadowg seadowg removed their request for review May 20, 2024 15:29
@seadowg
Copy link
Member

seadowg commented May 20, 2024

Removing myself as a reviewer until we've merged getodk/xforms-spec#311

@seadowg seadowg self-requested a review May 21, 2024 08:53
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)")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@lognaturel lognaturel requested a review from seadowg May 21, 2024 15:34
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.

Allow geo functions to take string arguments
3 participants