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.

Sign up for GitHub

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

Fixed ${} references in predicates not being expanded using current()… #536

Merged
merged 8 commits into from
May 18, 2021
Merged

Fixed ${} references in predicates not being expanded using current()… #536

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ebf664e
Fixed ${} references in predicates not being expanded using current()…
gushil Apr 26, 2021
4a6e80b
Expand refs using current() even if preceded by whitespace
gushil Apr 26, 2021
ce7c640
Added checking if ${name} is in a predicate / bracketed string
gushil May 17, 2021
f5d14b0
Added test for multiple predicate and ${name} expression after instance
gushil May 17, 2021
d8321de
Merge remote-tracking branch 'upstream/master' into fix-531
gushil May 17, 2021
1d9e058
Merge branch 'fix-531' of github.com:gushil/pyxform into fix-531
gushil May 17, 2021
0a61859
Remove unnecessary loop checking
gushil May 18, 2021
91e30e2
Remove unnecessary parenthesis in test
gushil May 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyxform/survey.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,7 @@ def _var_repl_function(
is_indexed_repeat = matchobj.string.find("indexed-repeat(") > -1
indexed_repeat_regex = re.compile(r"indexed-repeat\([^)]+\)")
function_args_regex = re.compile(r"\b[^()]+\((.*)\)$")
instance_regex = re.compile(r"instance\([^)]+\S+")
instance_regex = re.compile(r"instance\([^)]+.+")

def _in_secondary_instance_predicate():
"""
Expand Down
100 changes: 100 additions & 0 deletions pyxform/tests_v1/test_repeat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,6 +779,106 @@ def test_repeat_with_reference_path_in_predicate_uses_current(self,):
],
)

def test_repeat_with_reference_path_with_spaces_in_predicate_uses_current(self,):
"""
Test relative path expansion using current if reference path (with whitespaces before/after an operator of ${name}) is inside a predicate
"""
xlsform_md = """
| survey | | | | |
| | type | name | label | calculation |
| | xml-external | item | | |
| | calculate | pos1 | | position(..) |
| | begin repeat | rep5 | | |
| | calculate | pos5 | | position(..) |
| | calculate | item5 | | instance('item')/root/item[index= ${pos5} and ${pos1} = 1]/label |
| | calculate | item6 | | instance('item')/root/item[index =${pos5} and ${pos1} = 1]/label |
| | calculate | item7 | | instance('item')/root/item[index = ${pos5} and ${pos1} = 1]/label |
| | end repeat | | | |
"""
self.assertPyxformXform(
name="data",
md=xlsform_md,
xml__contains=[
"""<bind calculate="instance('item')/root/item[index= current()/../pos5 and /data/pos1 = 1]/label" nodeset="/data/rep5/item5" type="string"/>""", # noqa pylint: disable=line-too-long
"""<bind calculate="instance('item')/root/item[index = current()/../pos5 and /data/pos1 = 1]/label" nodeset="/data/rep5/item6" type="string"/>""", # noqa pylint: disable=line-too-long
"""<bind calculate="instance('item')/root/item[index = current()/../pos5 and /data/pos1 = 1]/label" nodeset="/data/rep5/item7" type="string"/>""", # noqa pylint: disable=line-too-long
],
)

def test_repeat_with_reference_path_in_a_method_with_spaces_in_predicate_uses_current(
self,
):
"""
Test relative path expansion using current if reference path in a method (with whitespaces before/after an operator of ${name}) is inside apredicate
"""
xlsform_md = """
| survey | | | | |
| | type | name | label | calculation |
| | xml-external | item | | |
| | calculate | pos1 | | position(..) |
| | begin repeat | rep5 | | |
| | calculate | pos5 | | position(..) |
| | calculate | item5 | | instance('item')/root/item[index=number(1+ ${pos5} and ${pos1} = 1]/label |
| | end repeat | | | |
"""
self.assertPyxformXform(
name="data",
md=xlsform_md,
xml__contains=[
"""<bind calculate="instance('item')/root/item[index=number(1+ current()/../pos5 and /data/pos1 = 1]/label" nodeset="/data/rep5/item5" type="string"/>""" # noqa pylint: disable=line-too-long
],
)

def test_repeat_with_reference_path_with_spaces_in_predicate_with_parenthesis_uses_current(
self,
):
"""
Test relative path expansion using current if reference path (with whitespaces before/after an operator of ${name}) is inside a predicate with parenthesis
"""
xlsform_md = """
| survey | | | | |
| | type | name | label | calculation |
| | xml-external | item | | |
| | calculate | pos1 | | position(..) |
| | begin repeat | rep5 | | |
| | calculate | pos5 | | position(..) |
| | calculate | item5 | | (instance('item')/root/item)[index = ${pos5} and ${pos1} = 1]/label |
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this use of parentheses valid in XPath? Mostly curious about what prompted this test!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, my bad. That comes from previous (non committed locally created) test. Will be fixed in the next commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to test reference path inside a predicate (bracketed) that includes reference in a repeat and reference outside a repeat. Is it acceptable test? Do you have any other test suggestion to achieve that?

Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the expression looks fine to me, I was just unclear on the significance of the parens! Agreed having a ref in the repeat and one outside is a good idea. 👍

Copy link

Choose a reason for hiding this comment

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

@lognaturel - I use patterns like this when counting nodes and then iterating over them in a repeating group.

So starting with something like:
count(instance('item')/root/group/item[name = 'test'])

I'd keep the parentheses the same and add an ordinal predicate for my repeating group position counter:
(instance('item')/root/group/item[name = 'test'])[position() = ${pos}]/value

My best recollection is that this was only needed when the data didn't repeat solely at the item node itself but rather at nodes further up the hierarchy (like group in this example).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the parens do make sense in that case. There's no need for them in (instance('item')/root/item)[index = ${pos5} and ${pos1} = 1]/label but I guess they're ok around any nodeset expression.

| | end repeat | | | |
"""
self.assertPyxformXform(
name="data",
md=xlsform_md,
xml__contains=[
"""<bind calculate="(instance('item')/root/item)[index = current()/../pos5 and /data/pos1 = 1]/label" nodeset="/data/rep5/item5" type="string"/>""" # noqa pylint: disable=line-too-long
],
)

def test_repeat_with_reference_path_with_whitespaces_in_multiple_predicate_uses_current(
self,
):
"""
Test relative path expansion using current if reference path (with whitespaces before/after an operator of ${name}) is inside multiple predicate
"""
xlsform_md = """
| survey | | | | |
| | type | name | label | calculation |
| | xml-external | item | | |
| | calculate | pos1 | | position(..) |
| | begin repeat | rep5 | | |
| | calculate | pos5 | | position(..) |
| | calculate | item5 | | join(' ', instance('item')/root/item)[index = ${pos5} and ${pos1} = 1]/label) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR, but just FYI, this test shows how hard it would be (have been) to implement #525. Since join() expects a node-set attribute but ${pos5} in this case has to be relative nonetheless since it used as a position/index....

| | calculate | item6 | | translate(instance('item')/root/item)[index = ${pos5} and ${pos1} = 1]/label, ',', ' ') |
| | end repeat | | | |
"""
self.assertPyxformXform(
name="data",
md=xlsform_md,
xml__contains=[
"""<bind calculate="join(' ', instance('item')/root/item)[index = current()/../pos5 and /data/pos1 = 1]/label)" nodeset="/data/rep5/item5" type="string"/>""", # noqa pylint: disable=line-too-long
"""<bind calculate="translate(instance('item')/root/item)[index = current()/../pos5 and /data/pos1 = 1]/label, ',', ' ')" nodeset="/data/rep5/item6" type="string"/>""", # noqa pylint: disable=line-too-long
],
)

def test_relative_path_expansion_not_using_current_if_reference_path_is_predicate_but_not_in_a_repeat(
self,
):
Expand Down