From d4e6d591227964876b701ce7d57350bbe70d4752 Mon Sep 17 00:00:00 2001 From: Ukang'a Dickson Date: Wed, 2 Jan 2019 23:43:01 +0300 Subject: [PATCH 1/9] Apply @property to SurveyElement.__name__(). Class method __name_ should be a property. --- pyxform/survey_element.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pyxform/survey_element.py b/pyxform/survey_element.py index 720fbed22..830ae76aa 100644 --- a/pyxform/survey_element.py +++ b/pyxform/survey_element.py @@ -1,4 +1,5 @@ import json + from pyxform import constants from pyxform.utils import is_valid_xml_tag, node, unicode from pyxform.xls2json import print_pyobj_to_json @@ -69,6 +70,7 @@ def __getattr__(self, key): return _overlay(over, under) raise AttributeError(key) + @property def __name__(self): return "SurveyElement" From 621863a2650ce4c671c6dd4afc6db0ff8ec5c52e Mon Sep 17 00:00:00 2001 From: Ukang'a Dickson Date: Wed, 2 Jan 2019 23:44:25 +0300 Subject: [PATCH 2/9] Make SurveyElement hashable. Will allow use of lru_cache devorator. --- pyxform/survey_element.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pyxform/survey_element.py b/pyxform/survey_element.py index 830ae76aa..4a43462ca 100644 --- a/pyxform/survey_element.py +++ b/pyxform/survey_element.py @@ -70,6 +70,9 @@ def __getattr__(self, key): return _overlay(over, under) raise AttributeError(key) + def __hash__(self): + return hash(id(self)) + @property def __name__(self): return "SurveyElement" From 41f2c4b7ee1f2cceb19ff45c53dcb5705855d8bb Mon Sep 17 00:00:00 2001 From: Ukang'a Dickson Date: Wed, 2 Jan 2019 23:50:53 +0300 Subject: [PATCH 3/9] Apply lru_cache() decorator to survey.is_a_parent() function. This reduces the time by more than half i.e 46s down to 16s. It is still 4 times more than v0.11.5. Fix https://github.com/XLSForm/pyxform/issues/247 --- pyxform/survey.py | 6 ++++++ requirements.pip | 1 + 2 files changed, 7 insertions(+) diff --git a/pyxform/survey.py b/pyxform/survey.py index 931659eca..385ee89e1 100644 --- a/pyxform/survey.py +++ b/pyxform/survey.py @@ -9,6 +9,10 @@ import xml.etree.ElementTree as ETree from collections import defaultdict from datetime import datetime +try: + from functools import lru_cache +except ImportError: + from functools32 import lru_cache from pyxform import constants from pyxform.errors import PyXFormError, ValidationError @@ -33,6 +37,7 @@ def register_nsmap(): register_nsmap() +@lru_cache(maxsize=None) def is_parent_a_repeat(survey, xpath): """ Returns the XPATH of the first repeat of the given xpath in the survey, @@ -50,6 +55,7 @@ def is_parent_a_repeat(survey, xpath): if any(repeats) else is_parent_a_repeat(survey, parent_xpath) +@lru_cache(maxsize=None) def share_same_repeat_parent(survey, xpath, context_xpath): """ Returns a tuple of the number of steps from the context xpath to the shared diff --git a/requirements.pip b/requirements.pip index 2f11e3359..902796d24 100644 --- a/requirements.pip +++ b/requirements.pip @@ -7,3 +7,4 @@ traceback2==1.4.0 # via unittest2 unicodecsv==0.14.1 unittest2==1.1.0 xlrd==1.1.0 +functools32==3.2.3.post2;python_version<"3.2" From 28b1947b0319827d6fe42b6cfe0c04a7aa6cf338 Mon Sep 17 00:00:00 2001 From: Ukang'a Dickson Date: Thu, 3 Jan 2019 12:25:36 +0300 Subject: [PATCH 4/9] Refactor: add survey.any_repeat() method Returns True on first repeat occurance and caches with lru_cache. Part fix for https://github.com/XLSForm/pyxform/issues/247 --- pyxform/survey.py | 17 ++++++++--------- pyxform/survey_element.py | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/pyxform/survey.py b/pyxform/survey.py index 385ee89e1..530eb1252 100644 --- a/pyxform/survey.py +++ b/pyxform/survey.py @@ -9,10 +9,6 @@ import xml.etree.ElementTree as ETree from collections import defaultdict from datetime import datetime -try: - from functools import lru_cache -except ImportError: - from functools32 import lru_cache from pyxform import constants from pyxform.errors import PyXFormError, ValidationError @@ -26,6 +22,11 @@ get_languages_with_bad_tags, node, unicode) from pyxform.validators import enketo_validate, odk_validate +try: + from functools import lru_cache +except ImportError: + from functools32 import lru_cache + def register_nsmap(): """Function to register NSMAP namespaces with ETree""" @@ -47,12 +48,10 @@ def is_parent_a_repeat(survey, xpath): if not parent_xpath: return False - repeats = [ - item for item in survey.iter_descendants() - if item.get_xpath() == parent_xpath and item.type == 'repeat'] + if survey.any_repeat(parent_xpath): + return parent_xpath - return parent_xpath \ - if any(repeats) else is_parent_a_repeat(survey, parent_xpath) + return is_parent_a_repeat(survey, parent_xpath) @lru_cache(maxsize=None) diff --git a/pyxform/survey_element.py b/pyxform/survey_element.py index 4a43462ca..c09293732 100644 --- a/pyxform/survey_element.py +++ b/pyxform/survey_element.py @@ -6,6 +6,11 @@ from pyxform.question_type_dictionary import QUESTION_TYPE_DICT from pyxform.errors import PyXFormError +try: + from functools import lru_cache +except ImportError: + from functools32 import lru_cache + def _overlay(over, under): if type(under) == dict: @@ -150,6 +155,16 @@ def iter_descendants(self): for f in e.iter_descendants(): yield f + @lru_cache(maxsize=None) + def any_repeat(self, parent_xpath): + """Return True if there ia any repeat in `parent_xpath`.""" + for item in self.iter_descendants(): + if item.get_xpath() == parent_xpath and \ + item.type == constants.REPEAT: + return True + + return False + def get_lineage(self): """ Return a the list [root, ..., self._parent, self] From 7fdb6745ebf52be92d4038eed7a7887c44d98ac0 Mon Sep 17 00:00:00 2001 From: Ukang'a Dickson Date: Thu, 3 Jan 2019 12:26:58 +0300 Subject: [PATCH 5/9] Only check share same repeat parent when the base parents are the same. --- pyxform/survey.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/pyxform/survey.py b/pyxform/survey.py index 530eb1252..a7b6b9c7e 100644 --- a/pyxform/survey.py +++ b/pyxform/survey.py @@ -722,14 +722,19 @@ def _var_repl_function(self, matchobj, context, use_current=False): raise PyXFormError(intro + " There are multiple survey elements" " with this name.") if context: - # if context xpath and target xpath fall under the same repeat use - # relative xpath referencing. - steps, ref_path = share_same_repeat_parent(self, self._xpath[name], - context.get_xpath()) - if steps: - ref_path = ref_path if ref_path.endswith(name) else "/%s" % name - prefix = " current()/" if use_current else " " - return prefix + "/".join([".."] * steps) + ref_path + " " + xpath, context_xpath = self._xpath[name], context.get_xpath() + # share same root i.e repeat_a from /data/repeat_a/... + if xpath.split('/')[2] == context_xpath.split('/')[2]: + # if context xpath and target xpath fall under the same repeat use + # relative xpath referencing. + steps, ref_path = share_same_repeat_parent(self, xpath, + context_xpath) + if steps: + ref_path = ref_path \ + if ref_path.endswith(name) else "/%s" % name + prefix = " current()/" if use_current else " " + + return prefix + "/".join([".."] * steps) + ref_path + " " return " " + self._xpath[name] + " " From b30ebbb9b0a93401bc50d193fff4e95d02e5f2d7 Mon Sep 17 00:00:00 2001 From: Ukang'a Dickson Date: Thu, 3 Jan 2019 14:10:50 +0300 Subject: [PATCH 6/9] Add python 2.7 required functools32 to setup.py. --- setup.py | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/setup.py b/setup.py index f8bbbaca2..bb24ae37e 100644 --- a/setup.py +++ b/setup.py @@ -1,7 +1,19 @@ # -*- coding: utf-8 -*- """Pyxform - Python library that converts XLSForms to XForms. """ -from setuptools import setup, find_packages +import sys + +from setuptools import find_packages, setup + +REQUIRES = [ + 'xlrd>=1.1.0', + 'unicodecsv>=0.14.1', + 'formencode', + 'unittest2', +] +if sys.version_info < (3, 2): + REQUIRES.append('functools32==3.2.3.post2') + setup( name='pyxform', @@ -30,12 +42,7 @@ url='http://pypi.python.org/pypi/pyxform/', description='A Python package to create XForms for ODK Collect.', long_description=open('README.rst', 'rt').read(), - install_requires=[ - 'xlrd>=1.1.0', - 'unicodecsv>=0.14.1', - 'formencode', - 'unittest2', - ], + install_requires=REQUIRES, entry_points={ 'console_scripts': [ 'xls2xform=pyxform.xls2xform:main_cli', From 6ccf57a54d066ea1b6ae1b848e960393b4521fc4 Mon Sep 17 00:00:00 2001 From: Ukang'a Dickson Date: Thu, 3 Jan 2019 14:41:22 +0300 Subject: [PATCH 7/9] Update CHANGES.txt --- CHANGES.txt | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGES.txt b/CHANGES.txt index ad06d880e..37ae0b4a0 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,6 +1,15 @@ Pyxform Changelog -v0.12.0, 2018 +Unreleased, 2019 01 [xx] + -- Performance Regression fix on using relative paths. + Issue #247, https://github.com/XLSForm/pyxform/issues/247 + [ukanga] + + -- Additional appearance values are supported with table-list. + Issue #61, https://github.com/XLSForm/pyxform/issues/61 + [KeynesYouDigIt] + +v0.12.0, 2018 12 09 -- Upgrade to ODK Validate v1.10.3 Issue #225, https://github.com/XLSForm/pyxform/issue/225 [ukanga] From 4380bf5b3055cfd88b56d8c20a5bc32f738cc0a2 Mon Sep 17 00:00:00 2001 From: Ukang'a Dickson Date: Sat, 5 Jan 2019 01:02:25 +0300 Subject: [PATCH 8/9] Do not apply relative path to indexed-repeat() expressions. --- pyxform/survey.py | 7 ++++--- pyxform/tests_v1/test_repeat.py | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/pyxform/survey.py b/pyxform/survey.py index a7b6b9c7e..57f1f0014 100644 --- a/pyxform/survey.py +++ b/pyxform/survey.py @@ -721,12 +721,13 @@ def _var_repl_function(self, matchobj, context, use_current=False): if self._xpath[name] is None: raise PyXFormError(intro + " There are multiple survey elements" " with this name.") - if context: + if context and not (context['type'] == 'calculate' and + 'indexed-repeat' in context['bind']['calculate']): xpath, context_xpath = self._xpath[name], context.get_xpath() # share same root i.e repeat_a from /data/repeat_a/... if xpath.split('/')[2] == context_xpath.split('/')[2]: - # if context xpath and target xpath fall under the same repeat use - # relative xpath referencing. + # if context xpath and target xpath fall under the same + # repeat use relative xpath referencing. steps, ref_path = share_same_repeat_parent(self, xpath, context_xpath) if steps: diff --git a/pyxform/tests_v1/test_repeat.py b/pyxform/tests_v1/test_repeat.py index f07caf165..d94a95c2d 100644 --- a/pyxform/tests_v1/test_repeat.py +++ b/pyxform/tests_v1/test_repeat.py @@ -105,7 +105,7 @@ def test_calculate_relative_path(self): title="Paths in a calculate within a repeat are relative.", md=""" | survey | | | | | - | | type | name | label | calculate | + | | type | name | label | calculation | | | begin repeat | rep | | | | | select_one crop_list | crop | Select | | | | text | a | Verify | name = ${crop} | @@ -155,3 +155,34 @@ def test_choice_filter_relative_path(self): # pylint: disable=invalid-name """""", # noqa pylint: disable=line-too-long ], ) + + def test_indexed_repeat_relative_path(self): + """Test relative path not used with indexed-repeat().""" + self.assertPyxformXform( + name="data", + title="Paths in a calculate within a repeat are relative.", + md=""" + | survey | | | | | + | | type | name | label | calculation | + | | begin repeat | rep | | | + | | begin repeat | rep2 | | | + | | select_one crop_list | crop | Select | | + | | text | a | Verify | | + | | begin group | group | | | + | | text | b | Verify | | + | | end group | | | | + | | end repeat | | | | + | | calculate | c1 | | indexed-repeat(${a}, ${rep2}, 1) | + | | end repeat | | | | + | | | | | | + | | | | | | + | choices | | | | | + | | list name | name | label | | + | | crop_list | maize | Maize | | + | | crop_list | beans | Beans | | + | | crop_list | kale | Kale | | + """, # noqa pylint: disable=line-too-long + model__contains=[ + """""", # noqa pylint: disable=line-too-long + ], + ) From b499446841c50937071ec321ed452e52faa09e0c Mon Sep 17 00:00:00 2001 From: Ukang'a Dickson Date: Sat, 5 Jan 2019 01:20:18 +0300 Subject: [PATCH 9/9] Code cleanup --- pyxform/survey.py | 54 ++++++++++++++++++++++++++++------------------- setup.py | 2 +- 2 files changed, 33 insertions(+), 23 deletions(-) diff --git a/pyxform/survey.py b/pyxform/survey.py index 57f1f0014..bd9d9d944 100644 --- a/pyxform/survey.py +++ b/pyxform/survey.py @@ -225,7 +225,8 @@ def _generate_static_instances(list_name, choice_list): @staticmethod def _get_dummy_instance(): """Instance content required by ODK Validate for select inputs.""" - return node("root", node("item", node("name", "_"), node("label", "_"))) + return node("root", + node("item", node("name", "_"), node("label", "_"))) @staticmethod def _generate_external_instances(element): @@ -345,8 +346,8 @@ def _generate_instances(self): Validation and business rules for output of instances: - - xml-external item name must be unique across the XForm and the form is - considered invalid if there is a duplicate name. This differs from + - xml-external item name must be unique across the XForm and the form + is considered invalid if there is a duplicate name. This differs from other item types which allow duplicates if not in the same group. - for all instance sources, if the same instance name is encountered, the following rules are used to allow re-using instances but prevent @@ -387,13 +388,14 @@ def _generate_instances(self): if i.name in seen.keys() and seen[i.name].src != i.src: # Instance id exists with different src URI -> error. msg = "The same instance id will be generated for different " \ - "external instance source URIs. Please check the form. " \ - "Instance name: '{i}', Existing type: '{e}', " \ + "external instance source URIs. Please check the form." \ + " Instance name: '{i}', Existing type: '{e}', " \ "Existing URI: '{iu}', Duplicate type: '{d}', " \ - "Duplicate URI: '{du}', Duplicate context: '{c}'.".format( - i=i.name, iu=seen[i.name].src, e=seen[i.name].type, - d=i.type, du=i.src, c=i.context - ) + "Duplicate URI: '{du}', Duplicate context: '{c}'."\ + .format( + i=i.name, iu=seen[i.name].src, e=seen[i.name].type, + d=i.type, du=i.src, c=i.context + ) raise PyXFormError(msg) elif i.name in seen.keys() and seen[i.name].src == i.src: # Instance id exists with same src URI -> ok, don't duplicate. @@ -418,7 +420,8 @@ def xml_model(self): model_children += list(self._generate_instances()) model_children += self.xml_bindings() - if self.submission_url or self.public_key or self.auto_send or self.auto_delete: + if (self.submission_url or self.public_key or self.auto_send or + self.auto_delete): submission_attrs = dict() if self.submission_url: submission_attrs["action"] = self.submission_url @@ -490,16 +493,20 @@ def _setup_choice_translations(name, choice_value, itext_id): self._translations, [media_type_or_language, itext_id, 'long'], value) - self._translations = defaultdict(dict) # pylint: disable=attribute-defined-outside-init + self._translations = defaultdict(dict) # pylint: disable=W0201 for element in self.iter_descendants(): for d in element.get_translations(self.default_language): if 'guidance_hint' in d['path']: hint_path = d['path'].replace('guidance_hint', 'hint') - self._translations[d['lang']][hint_path] = self._translations[d['lang']].get(hint_path, {}) - self._translations[d['lang']][hint_path].update({"guidance": d['text']}) + self._translations[d['lang']][hint_path] = \ + self._translations[d['lang']].get(hint_path, {}) + self._translations[d['lang']][hint_path].update( + {"guidance": d['text']}) else: - self._translations[d['lang']][d['path']] = self._translations[d['lang']].get(d['path'], {}) - self._translations[d['lang']][d['path']].update({"long": d['text']}) + self._translations[d['lang']][d['path']] = \ + self._translations[d['lang']].get(d['path'], {}) + self._translations[d['lang']][d['path']].update( + {"long": d['text']}) # This code sets up translations for choices in filtered selects. for list_name, choice_list in self.choices.items(): @@ -508,7 +515,8 @@ def _setup_choice_translations(name, choice_value, itext_id): itext_id = '-'.join(['static_instance', list_name, str(idx)]) if isinstance(choice_value, dict): - _setup_choice_translations(name, choice_value, itext_id) + _setup_choice_translations(name, choice_value, + itext_id) elif name == 'label': self._add_to_nested_dict( self._translations, @@ -543,7 +551,7 @@ def _setup_media(self): It matches the xform nesting order. """ if not self._translations: - self._translations = defaultdict(dict) # pylint: disable=attribute-defined-outside-init + self._translations = defaultdict(dict) # pylint: disable=W0201 for survey_element in self.iter_descendants(): @@ -630,12 +638,13 @@ def itext(self): if media_type == "guidance": itext_nodes.append( node("value", value, - form='guidance', - toParseString=output_inserted) + form='guidance', + toParseString=output_inserted) ) else: - itext_nodes.append( - node("value", value, toParseString=output_inserted) + itext_nodes.append( + node("value", value, + toParseString=output_inserted) ) continue @@ -743,10 +752,11 @@ def insert_xpaths(self, text, context, use_current=False): """ Replace all instances of ${var} with the xpath to var. """ - bracketed_tag = r"\$\{(.*?)\}" def _var_repl_function(matchobj): return self._var_repl_function(matchobj, context, use_current) + bracketed_tag = r"\$\{(.*?)\}" + return re.sub(bracketed_tag, _var_repl_function, unicode(text)) def _var_repl_output_function(self, matchobj, context): diff --git a/setup.py b/setup.py index bb24ae37e..cee8a390e 100644 --- a/setup.py +++ b/setup.py @@ -11,10 +11,10 @@ 'formencode', 'unittest2', ] + if sys.version_info < (3, 2): REQUIRES.append('functools32==3.2.3.post2') - setup( name='pyxform', version='0.12.0',