Add substring-before() and substring-after() XPath functions #393
Conversation
Add substring-before() and substring-after()
There was a problem hiding this comment.
Thanks, @tiritea! I made some comments about the changes you've made.
I'm also missing some tests for the new functions. Please, add these lines after the last test of the substr() function in XPathEvalTest.java#252:
testEval("substring-before('hello','l')", "he");
testEval("substring-before('hello','q')", "");
testEval("substring-before('hello','')", "");
testEval("substring-before('','')", "");
testEval("substring-before('','q')", "");
testEval("substring-after('hello','l')", "lo");
testEval("substring-after('hello','q')", "");
testEval("substring-after('hello','')", "hello");
testEval("substring-after('','')", "");
testEval("substring-after('','q')", "");| @@ -344,6 +344,16 @@ public Object eval (DataInstance model, EvaluationContext evalContext) { | |||
| } | |||
| } else if (name.equals("substr") && (args.length == 2 || args.length == 3)) { | |||
| return substring(argVals[0], argVals[1], args.length == 3 ? argVals[2] : null); | |||
| } else if (name.equals("substring-before") && args.length == 2) { | |||
| String str = toString(argVals[0]); | |||
There was a problem hiding this comment.
I'd like to suggest some changes:
- MDN suggests
haystackandneedleargument names. I think those names do a great work of explaining what's going on. - Use
needleIndexinstead ofpos. It's not abbreviated, it's more expressive (you're calling anindexOfmethod, therefore, the result should be an index), and it's explicit about its relationship with theneedle. - Remove the parenthesis (redundant) in the
returnline. - I think the comment should be moved to a line before the return and explicitly say that the XPath reference states that an empty string should be returned when the
needleisn't found. Otherwise, it could look like some arbitrary decision on our side.
String haystack = toString(argVals[0]);
String needle = toString(argVals[1]);
int needleIndex = haystack.indexOf(needle);
// XPath reference states that we should return the empty string when we don't find the needle in the haystack
return needleIndex >= 0 ? haystack.substring(0, needleIndex) : "";There was a problem hiding this comment.
Thanks for your suggestions. Done.
There was a problem hiding this comment.
I think the needle in a haystack metaphor is dumb, and phooey on MDN for using it.
https://dictionary.cambridge.org/us/dictionary/english/a-needle-in-a-haystack
There was a problem hiding this comment.
I have no opinion either way... But I do notice that javaRosa does sometime errs on the side of 'verbosity', so its not completely out of character. eg
return GeoUtils.calculateAreaOfGPSPolygonOnEarthInSquareMeters(latLongs);
;-)
There was a problem hiding this comment.
Ha ha.
Verbosity is a different dimension from frivolity or suitability, though.
Was it a coincidence that you picked something I named? ;-)
I don’t feel strongly enough about the needle and a haystack to ask you to change it again.
There was a problem hiding this comment.
Was it a coincidence that you picked something I named?
D'oh! :-)
There was a problem hiding this comment.
I think the needle in a haystack metaphor is dumb, and phooey on MDN for using it.
Huh, I didn't know about that. This seemed to be an improvement over arg0 and arg1, but now I'm hesitating. Do you have any better alternatives, @dcbriccetti?
Codecov Report
@@ Coverage Diff @@
## master #393 +/- ##
============================================
+ Coverage 48.29% 48.31% +0.01%
- Complexity 2868 2871 +3
============================================
Files 239 239
Lines 13506 13516 +10
Branches 2621 2625 +4
============================================
+ Hits 6523 6530 +7
Misses 6148 6148
- Partials 835 838 +3
Continue to review full report at Codecov.
|
|
I believe I've made the necessary changes and its ready, but as its my first ODK PR please let me know if I've omitted anything github-wise. Thnx. |
|
@tiritea, @dcbriccetti, I've approved the PR, although maybe we should give a last thought to the arg0/arg1/haystack/needle conundrum before merging this. |
|
@ggalmazor , @dcbriccetti - it'll let you guys call the shots wrt javaRosa variable name best-practices. I'm happy with either str/substr/pos or haystack/needle/needleIndex |
|
I much prefer str/substr/pos to the haystack business. And both, of course, to arg0.... |
|
I'm happy to change it back. But it will mean @ggalmazor owes me a beer... ;-) |
|
Heh heh. Although his English is excellent—his writing, especially—we can forgive @ggalmazor for not knowing the connotations of “needle in a haystack”. I wonder what the nutty https://developer.mozilla.org/en-US/docs/Web/XPath/Functions/substring-before developer/documenter was thinking, though. One should not have such a negative attitude about a search. ;-) |
|
@ggalmazor - Given that finding said 'needle' is actually incredibly simple when every piece of straw happens to lined up adjacently, perhaps we should adopt a more appropriate idiom here, say "Ser pan comido"? :-) [I'll change it back to str/substr/pos |
|
Ha ha! |
|
Ha ha indeed! In Spanish we say ja ja, by the way. |
|
@tiritea Can you please send in a PR that documents this change? The issue is at getodk/docs#935. |
Closes https://forum.opendatakit.org/t/add-xpath-substring-before-and-substring-after-functions/16863
###What has been done to verify that this works as intended?
tested under Android Studio simulator running ODK Collect v1.18.0 (dirty), using test examples described on http://www.xsltfunctions.com/xsl/fn_substring-before.html and http://www.xsltfunctions.com/xsl/fn_substring-after.html, and compared results running same against Enketo.
###Why is this the best possible solution? Were any other approaches considered?
Implements XPath standardized functions (which is arguably best solution). Implements identical XPath behavior as exists already in Enketo and libxml2.
###Are there any risks to merging this code? If so, what are they?
Introduces entirely new XPath functions and does not touch or modify any existing javaRosa function or behavior. Can only be used by new forms that exploit these new functions in their XPath expressions, so no risk to existing forms.