When indexed-repeat is used within a repeat, 1st, 2nd, 4th, and 6th a…

[gushil]

…rgument not using relative path.

Closes #
Fix #484

Why is this the best possible solution? Were any other approaches considered?

The one that works and passed tests.

What are the regression risks?

All tests passed, including new ones.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No

Before submitting this PR, please make sure you have:

• included test cases for core behavior and edge cases in tests_v1
• run nosetests and verified all tests pass
• run black pyxform to format code
• verified that any code or assets from external sources are properly credited in comments

[MartijnR]

Thanks @gushil!

I checked the tests and this all looks good. I also double-checked the output when indexed-repeat is used in a constraint, relevant and required, and this is also correct.

I glanced at the code a little bit, and all the if-statements seemed unexpected to me. However, I have no understanding of the intricacies of how the current ${node} references are processed when referring to nodes inside repeats from that repeat. So, I am hoping this makes immediate sense to @lognaturel...

[lognaturel]

all the if-statements seemed unexpected to me.

Agreed, this looks tough to maintain and evolve. Perhaps it's the only reasonable solution but it would really help to have a narrative on what alternatives you considered and why you think this is the best/only approach. Otherwise the only way I can provide meaningful review is to solve the problem myself all over again. It appears that you are explicitly identifying all the different places that an indexed-repeat function could appear. Is there not a single place where that could be performed regardless of what expression indexed-repeat is in? At the very least, this logic should be grouped in a function with a clear comment on its purpose.

I think it's important to have some tests with expressions that use the indexed-repeat result. For example:

• 7 * indexed-repeat(${age}, ${family}, 1, ${person}, 2)
• concat(indexed-repeat(${name}, ${family}, 1, ${person}, 2), indexed-repeat(${age}, ${family}, 1, ${person}, 2))
• ${age} > indexed-repeat(${age}, ${family}, 1, ${person}, 2)

The last one is particularly interesting because within a repeat ${age} needs to be relative.

[gushil]

@lognaturel @MartijnR
Thanks for the reviews!

I agree with your comment about the code looks quite difficult to understand. I try to not moving the code too much, but it looks like it made it difficult to understand and too many conditional repetitions.

Basically, I want to add conditions related to this issue in addition to the existing relative path condition:

not ( context["type"] == "calculate" and "indexed-repeat" in context["bind"]["calculate"] )

But it looks like, some of the conditions related to this issue are covered in the existing condition, so I break the current condition and add conditions related to this issue.

Another problem I found is that I cannot tell if this name to be replaced is in indexed-repeat without checking all conditions (context["bind"]["calculate"] and context["default"]), so I made those checks.

I will try to make it simpler and add some comments to it.
I'm open to any suggestions.

Thanks

[gushil]

Hi, @lognaturel @MartijnR

Code has been refactored and all additional tests passed.

Hopefully, the current implementation is simpler and easy to understand. I've added some comments too.

Thanks.

[lognaturel]

Thanks for the good rework, @gushil. I'm comfortable with it now.

One case I've found that isn't handled is if the indexed-repeat call is nested in another call and there's a relative reference parameter after. For example, consider concat(indexed-repeat(${name}, ${family}, 1, ${person}, 2), ${name}) in a nested repeat. The output should be concat(indexed-repeat( /data/family/person/name , /data/family , 1, /data/family/person , 2), ../name ) but is concat(indexed-repeat( /data/family/person/name , /data/family , 1, /data/family/person , 2), /data/family/person/name ).

Putting the ${name} as the first parameter works: concat(${name}, indexed-repeat(${name}, ${family}, 1, ${person}, 2)) correctly produces concat( ../name , indexed-repeat( /data/family/person/name , /data/family , 1, /data/family/person , 2)).

[codecov-io]

Codecov Report

Merging #485 (c01d8e3) into master (4d9decd) will increase coverage by 0.15%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master     #485      +/-   ##
==========================================
+ Coverage   83.63%   83.78%   +0.15%     
==========================================
  Files          25       25              
  Lines        3641     3676      +35     
  Branches      846      855       +9     
==========================================
+ Hits         3045     3080      +35     
  Misses        452      452              
  Partials      144      144              

Impacted Files	Coverage Δ
pyxform/survey.py	92.94% <95.23%> (+0.52%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d9decd...c01d8e3. Read the comment docs.

[gushil]

Hi @lognaturel @MartijnR

Addressed @MartijnR reviews and fixed the code (and add a test too) to fix the bug that @lognaturel mentioned in the feedback.

Thanks.

[lognaturel]

👍

In general, doing argument parsing with regex (as opposed to a parser) scares me a bit but based on the tests, I think you've pulled it off. I think there are probably still cases like commas in quotes that could break but I think it's ok to wait and see whether it's something users actually end up needing.