-
Notifications
You must be signed in to change notification settings - Fork 136
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
When indexed-repeat is used within a repeat, 1st, 2nd, 4th, and 6th a… #485
Conversation
…rgument not using relative path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
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 I think it's important to have some tests with expressions that use the indexed-repeat result. For example:
The last one is particularly interesting because within a repeat |
@lognaturel @MartijnR 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:
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. Thanks |
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. |
Thanks for the good rework, @gushil. I'm comfortable with it now. One case I've found that isn't handled is if the Putting the |
…${name} as arguments
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Addressed @MartijnR reviews and fixed the code (and add a test too) to fix the bug that @lognaturel mentioned in the feedback. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
…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:
tests_v1
nosetests
and verified all tests passblack pyxform
to format code