Add XPath translate() function #395
Conversation
Codecov Report
@@ Coverage Diff @@
## master #395 +/- ##
============================================
+ Coverage 48.31% 48.35% +0.04%
- Complexity 2871 2875 +4
============================================
Files 239 239
Lines 13516 13530 +14
Branches 2625 2629 +4
============================================
+ Hits 6530 6543 +13
Misses 6148 6148
- Partials 838 839 +1
Continue to review full report at Codecov.
|
| if (fromPos == -1) { | ||
| // no mapping for this char, so just add it to result unchanged | ||
| result.append(from); | ||
| } else if (fromPos < toChars.length()) { |
There was a problem hiding this comment.
toChars.length() could be pre-computed before the for block to avoid computing it each loop iteration.
There was a problem hiding this comment.
Yes, I had thought about doing that, but then saw that this level of for loop optimization wasnt't being performed elsewhere in javaRosa; eg in toNumeric():
653 for (int i = 0; i < s.length(); i++) {...
So I assumed the preference was readability vs micro-optimization?
OK. I see what you are referring to now.
There was a problem hiding this comment.
Done. Good catch. Thnx.
BTW I left the other potential candidate for this optimization - namely the for loop terminating expression - unchanged, for internal consistency with existing toNumeric().
| testEval("translate('hello','lo','LO')", "heLLO"); | ||
| testEval("translate('hello world','hello','')", "wrd"); | ||
| testEval("translate('hello wor,ld',' ,',', ')", "hello,wor ld"); | ||
| testEval("translate('2019/01/02','/','-')", "2019-01-02"); |
There was a problem hiding this comment.
Neat! Thanks for thinking about the tests! :)
|
It'd be good for this to be documented in the docs. The bare minimum should be an issue is filed at https://github.com/opendatakit/docs/issues/new! |
|
My plan is to doc this translate() together with substring-before() and substring-after() in one PR. Hence I was waiting for this one to be approved before proceeding with updating docs. But I can open a doc issue in mean time. |
|
Are we waiting on me for anything before merging this? If we can get it in before 16th we might be able to rev javaRosa to including this [and the new substring-before() & substring-after() functions] for next Collect release. |
|
Normally we would try the PR in Collect as a sort of QA phase before merging. Do you have any form we could use to test this? In any case, I'd say we merge it because It's a narrow changeset and it has automated tests. If no one complains by the end of day I can merge this. |
Only the one I posted to the Forum: https://forum.opendatakit.org/t/add-xpath-translate-function/17017/3 Sorry. I didn't mean to appear impatient. I just wanted to make sure there wasn't something waiting on me to move it along... I think I covered all same bases as my other PR that got merged, but then I've already violated numerous procedural 'protocols' on both! :-) |
|
Sure, don't worry! I think you're right. I didn't remember the previous PR with the substring functions. Let's merge this one. |
You mean you don't remember the beer you owe me!!!??? Hurmph. ;-) |
Closes # https://forum.opendatakit.org/t/add-xpath-translate-function/17017
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_translate.html (and those added to testEval testcases), and compared results running same against Enketo.
Why is this the best possible solution? Were any other approaches considered?
Implements XPath standardized function (which is arguably best solution). Implements identical XPath behavior as exists already in Enketo and libxml2.
BTW, I also tried an implementation that avoided using a StringBuilder, and instead tried to exploit existing java String replace()/replaceAll() functions, but this ended up much messier, required multiple re-instantiations of String objects, and ultimately did not handle simultaneous interchanging translations correctly (eg 'x' --> 'y' and 'y' --> 'x').
Are there any risks to merging this code? If so, what are they?
Introduces entirely new XPath function and does not touch or modify any existing javaRosa function or behavior. Can only be used by new forms that exploit this new function in their XPath expressions, so no risk to existing forms.