Merge pull request #673 from lindsay-stevens/pyxform-660

660: nicer error about choices sheet when extension omitted in select_.._from_file

pyxform/aliases.py

@@ -18,6 +18,12 @@
     "loop": constants.LOOP,
     "looped group": constants.REPEAT,
 }
+select_from_file = {
+    "select_one_from_file": constants.SELECT_ONE,
+    "select_multiple_from_file": constants.SELECT_ALL_THAT_APPLY,
+    "select one from file": constants.SELECT_ONE,
+    "select multiple from file": constants.SELECT_ALL_THAT_APPLY,
+}
 select = {
     "add select one prompt using": constants.SELECT_ONE,
     "add select multiple prompt using": constants.SELECT_ALL_THAT_APPLY,
@@ -29,10 +35,7 @@
     "select_multiple": constants.SELECT_ALL_THAT_APPLY,
     "select all that apply": constants.SELECT_ALL_THAT_APPLY,
     "select_one_external": constants.SELECT_ONE_EXTERNAL,
-    "select_one_from_file": constants.SELECT_ONE,
-    "select_multiple_from_file": constants.SELECT_ALL_THAT_APPLY,
-    "select one from file": constants.SELECT_ONE,
-    "select multiple from file": constants.SELECT_ALL_THAT_APPLY,
+    **select_from_file,
     "rank": constants.RANK,
 }
 cascading = {

pyxform/entities/entities_parsing.py

@@ -125,11 +125,9 @@ def validate_entity_saveto(
 
 
 def validate_entities_columns(row: Dict):
-    expected = set(EC.value_list())
-    observed = set(row.keys())
-    extra = observed.difference(expected)
+    extra = {k: None for k in row.keys() if k not in EC.value_list()}
     if 0 < len(extra):
-        fmt_extra = ", ".join((f"'{k}'" for k in extra))
+        fmt_extra = ", ".join((f"'{k}'" for k in extra.keys()))
         msg = (
             f"The entities sheet included the following unexpected column(s): {fmt_extra}. "
             f"These columns are not supported by this version of pyxform. Please either: "

pyxform/validators/pyxform/select_from_file_params.py → pyxform/validators/pyxform/select_from_file.py

@@ -1,6 +1,8 @@
 import re
+from pathlib import Path
 
-from pyxform.constants import ROW_FORMAT_STRING
+from pyxform import aliases
+from pyxform.constants import EXTERNAL_INSTANCE_EXTENSIONS, ROW_FORMAT_STRING
 from pyxform.errors import PyXFormError
 
 VALUE_OR_LABEL_TEST_REGEX = re.compile(r"^[a-zA-Z_][a-zA-Z0-9\-_\.]*$")
@@ -42,3 +44,20 @@ def value_or_label_check(name: str, value: str, row_number: int) -> None:
         msg = value_or_label_format_msg(name=name, row_number=row_number)
         raise PyXFormError(msg)
     return None
+
+
+def validate_list_name_extension(
+    select_command: str, list_name: str, row_number: int
+) -> None:
+    """For select_from_file types, the list_name should end with a supported extension."""
+    list_path = Path(list_name)
+    if select_command in aliases.select_from_file and (
+        1 != len(list_path.suffixes)
+        or list_path.suffix not in EXTERNAL_INSTANCE_EXTENSIONS
+    ):
+        exts = ", ".join((f"'{e}'" for e in EXTERNAL_INSTANCE_EXTENSIONS))
+        raise PyXFormError(
+            ROW_FORMAT_STRING % row_number
+            + f" File name for '{select_command} {list_name}' should end with one of "
+            + f"the supported file extensions: {exts}"
+        )

pyxform/xls2json.py

@@ -23,7 +23,7 @@
 )
 from pyxform.errors import PyXFormError
 from pyxform.utils import PYXFORM_REFERENCE_REGEX, default_is_dynamic
-from pyxform.validators.pyxform import parameters_generic, select_from_file_params
+from pyxform.validators.pyxform import parameters_generic, select_from_file
 from pyxform.validators.pyxform.android_package_name import validate_android_package_name
 from pyxform.validators.pyxform.translations_checks import SheetTranslations
 from pyxform.xls2json_backends import csv_to_dict, xls_to_dict, xlsx_to_dict
@@ -1120,6 +1120,11 @@ def workbook_to_json(
                         + "List name not in external choices sheet: "
                         + list_name
                     )
+                select_from_file.validate_list_name_extension(
+                    select_command=parse_dict["select_command"],
+                    list_name=list_name,
+                    row_number=row_number,
+                )
                 if (
                     list_name not in choices
                     and select_type != constants.SELECT_ONE_EXTERNAL
@@ -1209,13 +1214,13 @@ def workbook_to_json(
                     )
 
                 if "value" in parameters.keys():
-                    select_from_file_params.value_or_label_check(
+                    select_from_file.value_or_label_check(
                         name="value",
                         value=parameters["value"],
                         row_number=row_number,
                     )
                 if "label" in parameters.keys():
-                    select_from_file_params.value_or_label_check(
+                    select_from_file.value_or_label_check(
                         name="label",
                         value=parameters["label"],
                         row_number=row_number,

tests/test_entities_create.py

@@ -418,7 +418,7 @@ def test_entities_columns__one_unexpected(self):
             |          | trees     | a     | !     |
             """,
             errored=True,
-            error_contains=[
+            error__contains=[
                 "The entities sheet included the following unexpected column(s): 'what'"
             ],
         )
@@ -434,7 +434,9 @@ def test_entities_columns__multiple_unexpected(self):
             |          | trees     | a     | !     | ?   |
             """,
             errored=True,
-            error_contains=[
-                "The entities sheet included the following unexpected column(s): 'what', 'why'"
+            error__contains=[
+                "The entities sheet included the following unexpected column(s):",
+                "'what'",
+                "'why'",
             ],
         )

tests/test_external_instances_for_selects.py

@@ -7,6 +7,7 @@
 import os
 from dataclasses import dataclass, field
 
+from pyxform import aliases
 from pyxform.constants import EXTERNAL_INSTANCE_EXTENSIONS
 from pyxform.errors import PyXFormError
 from pyxform.xls2xform import get_xml_path, xls2xform_convert
@@ -202,7 +203,7 @@ def test_param_value_and_label_validation(self):
         |        | type          | name    | label   | parameters |
         |        | {q} cities{e} | city    | City    | {p}        |
         """
-        from pyxform.validators.pyxform import select_from_file_params
+        from pyxform.validators.pyxform import select_from_file
 
         q_types = ("select_one_from_file", "select_multiple_from_file")
         good_params = (
@@ -235,7 +236,7 @@ def test_param_value_and_label_validation(self):
                         name = "value"
                         if "label" in param:
                             name = "label"
-                        msg = select_from_file_params.value_or_label_format_msg(
+                        msg = select_from_file.value_or_label_format_msg(
                             name=name, row_number=2
                         )
                         self.assertPyxformXform(
@@ -262,6 +263,28 @@ def test_param_value_case_preserved(self):
                     ],
                 )
 
+    def test_expected_error_message(self):
+        """Should get helpful error when select_from_file is missing a file extension."""
+        md = """
+        | survey |                      |      |       |
+        |        | type                 | name | label |
+        |        | {select} cities{ext} | city | City  |
+        """
+        types = aliases.select_from_file.keys()
+        exts = ("", ".exe", ".sav", ".pdf")
+        error = (
+            "File name for '{select} cities{ext}' should end with "
+            "one of the supported file extensions"
+        )
+        for select in types:
+            for ext in exts:
+                with self.subTest(msg=(select, ext)):
+                    self.assertPyxformXform(
+                        md=md.format(select=select, ext=ext),
+                        errored=True,
+                        error__contains=[error.format(select=select, ext=ext)],
+                    )
+
 
 class TestSelectOneExternal(PyxformTestCase):
     """
@@ -477,37 +500,3 @@ def test_external_choices_with_only_header__errors(self):
                 self.assertContains(
                     str(e), "should be an external_choices sheet in this xlsform"
                 )
-
-
-class TestInvalidExternalFileInstances(PyxformTestCase):
-    def test_external_other_extension_instances(self):
-        # re: https://github.com/XLSForm/pyxform/issues/30
-        self.assertPyxformXform(
-            name="epdf",
-            md="""
-            | survey |                                              |                |                |
-            |        | type                                         | name           | label          |
-            |        | select_one_from_file cities.pdf              | city           | City           |
-            |        | select_multiple_from_file neighbourhoods.pdf | neighbourhoods | Neighbourhoods |
-            """,  # noqa
-            errored=True,
-            error__contains=["should be a choices sheet in this xlsform"],
-        )
-
-    def test_external_choices_sheet_included_instances(self):
-        # re: https://github.com/XLSForm/pyxform/issues/30
-        self.assertPyxformXform(
-            name="epdf",
-            md="""
-            | survey |                                              |                |                |
-            |        | type                                         | name           | label          |
-            |        | select_one_from_file cities.pdf              | city           | City           |
-            |        | select_multiple_from_file neighbourhoods.pdf | neighbourhoods | Neighbourhoods |
-
-            | choices |
-            |         | list name | name  | label |
-            |         | fruits    | apple | Apple |
-            """,  # noqa
-            errored=True,
-            error__contains=["List name not in choices sheet: cities.pdf"],
-        )