Repository: XLSForm/pyxform (python)

Issue:
Duplicate choice name message is confusing
#310 introduced a `allow_choice_duplicates` setting. The error message when that setting is not set and a list has duplicate choice names is: "There does not seem to be a `allow_choice_duplicates` column header defined in your settings sheet. You must have set `allow_choice_duplicates` setting in your settings sheet to have duplicate choice list names in your choices sheet"

The message should lead with what is wrong with the form (a choice list has duplicate choice names).

I propose something like:
"One or more of your choice lists contains duplicate choice names. That means the choices with the same name will be impossible to differentiate in analysis. If this is intentional, you can set the `allow_choice_duplicates` setting. Read more at `<url once documented>`."

Having a URL means there can be an example which would be helpful to users who may not understand the problem with duplicate choice names.

It might also be helpful to identify the lists and/or the choices that are affected. https://github.com/XLSForm/pyxform/pull/310/files#diff-b29aeb282b118614b8333cdfc19f700dR494 attempts to do this but it should be revisited because it only happens if the `allow_choice_duplicates` is defined in the settings sheet but not set to yes|Yes.

Pull request:
Clarify duplicate choice error message
Closes #373

Introduces a clearer message for duplicate choices in the same list, linking out to documentation provided at https://github.com/XLSForm/xlsform.github.io/pull/184. 

For some reason, the case where no `allow_choice_duplicates` setting was defined was previously less informative than the case where the setting existed but was not set to `yes`. These now show the same message.

Patch:
diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py
index fca449e..c39b54a 100644
--- a/pyxform/xls2json.py
+++ b/pyxform/xls2json.py
@@ -500,34 +500,28 @@ def workbook_to_json(
         list_name_choices = [option.get("name") for option in options]
         if len(list_name_choices) != len(set(list_name_choices)):
             duplicate_setting = settings.get("allow_choice_duplicates")
-            if not duplicate_setting:
-                raise PyXFormError(
-                    "There does not seem to be"
-                    " a `allow_choice_duplicates`"
-                    " column header defined in your settings sheet."
-                    " You must have set `allow_choice_duplicates`"
-                    " setting in your settings sheet"
-                    " to have duplicate choice list names"
-                    " in your choices sheet"
-                )  # noqa
             for k, v in Counter(list_name_choices).items():
                 if v > 1:
-                    if duplicate_setting and duplicate_setting.capitalize() != "Yes":
-                        result = [
+                    if not duplicate_setting or duplicate_setting.capitalize() != "Yes":
+                        choice_duplicates = [
                             item
                             for item, count in Counter(list_name_choices).items()
                             if count > 1
                         ]
-                        choice_duplicates = " ".join(result)
 
                         if choice_duplicates:
                             raise PyXFormError(
-                                "On the choices sheet the choice list name"
-                                " '{}' occurs more than once."
-                                " You must have set `allow_choice_duplicates`"
-                                " setting in your settings sheet"
-                                " for this to be a valid measure".format(
-                                    choice_duplicates
+                                "The name column for the '{}' choice list contains these duplicates: {}. Duplicate names "
+                                "will be impossible to identify in analysis unless a previous value in a cascading "
+                                "select differentiates them. If this is intentional, you can set the "
+                                "allow_choice_duplicates setting to 'yes'. Read more: https://xlsform.org/choice-names.".format(
+                                    list_name,
+                                    ", ".join(
+                                        [
+                                            "'{}'".format(dupe)
+                                            for dupe in choice_duplicates
+                                        ]
+                                    ),
                                 )
                             )  # noqa
 


Test patch:
diff --git a/pyxform/tests_v1/test_fields.py b/pyxform/tests_v1/test_fields.py
index dc8b7b5..125dbae 100644
--- a/pyxform/tests_v1/test_fields.py
+++ b/pyxform/tests_v1/test_fields.py
@@ -42,81 +42,97 @@ class FieldsTests(PyxformTestCase):
             error__contains=["There are more than one survey elements named 'age'"],
         )
 
-    def test_duplicate_choice_list_without_settings(self):
+    def test_duplicate_choices_without_setting(self):
         self.assertPyxformXform(
             md="""
-            | survey  |                 |          |       |
-            |         | type            | name     | label |
-            |         | select_one list | S1       | s1    |
-            | choices |                 |          |       |
-            |         | list name       | name     | label  |
-            |         | list            | option a | a      |
-            |         | list            | option b | b      |
-            |         | list            | option b | c      |
+            | survey  |                 |          |          |
+            |         | type            | name     | label    |
+            |         | select_one list | S1       | s1       |
+            | choices |                 |          |          |
+            |         | list name       | name     | label    |
+            |         | list            | a        | option a |
+            |         | list            | b        | option b |
+            |         | list            | b        | option c |
             """,
             errored=True,
             error__contains=[
-                "There does not seem to be a"
-                " `allow_choice_duplicates` column header defined"
-                " in your settings sheet"
+                "The name column for the 'list' choice list contains these duplicates: 'b'"
             ],  # noqa
         )
 
-    def test_duplicate_choice_list_with_wrong_setting(self):
+    def test_multiple_duplicate_choices_without_setting(self):
         self.assertPyxformXform(
             md="""
-            | survey  |                 |          |       |
-            |         | type            | name     | label |
-            |         | select_one list | S1       | s1    |
-            | choices |                 |          |       |
-            |         | list name       | name     | label  |
-            |         | list            | option a | a      |
-            |         | list            | option b | b      |
-            |         | list            | option b | c      |
-            | settings |                |          |        |
+            | survey  |                 |          |          |
+            |         | type            | name     | label    |
+            |         | select_one list | S1       | s1       |
+            | choices |                 |          |          |
+            |         | list name       | name     | label    |
+            |         | list            | a        | option a |
+            |         | list            | a        | option b |
+            |         | list            | b        | option c |
+            |         | list            | b        | option d |
+            """,
+            errored=True,
+            error__contains=[
+                "The name column for the 'list' choice list contains these duplicates: 'a', 'b'"
+            ],  # noqa
+        )
+
+    def test_duplicate_choices_with_setting_not_set_to_yes(self):
+        self.assertPyxformXform(
+            md="""
+            | survey  |                 |          |          |
+            |         | type            | name     | label    |
+            |         | select_one list | S1       | s1       |
+            | choices |                 |          |          |
+            |         | list name       | name     | label    |
+            |         | list            | a        | option a |
+            |         | list            | b        | option b |
+            |         | list            | b        | option c |
+            | settings |                |          |          |
             |          | id_string    | allow_choice_duplicates   |
-            |          | Duplicates   | True                       |
+            |          | Duplicates   | Bob                       |
             """,
             errored=True,
             error__contains=[
-                "On the choices sheet the choice list name"
-                " 'option b' occurs more than once."
+                "The name column for the 'list' choice list contains these duplicates: 'b'"
             ],  # noqa
         )
 
-    def test_duplicate_choice_list_with_setting(self):
+    def test_duplicate_choices_with_allow_choice_duplicates_setting(self):
         md = """
-            | survey  |                 |          |       |
-            |         | type            | name     | label |
-            |         | select_one list | S1       | s1    |
-            | choices |                 |          |       |
-            |         | list name       | name     | label  |
-            |         | list            | option a | a      |
-            |         | list            | option b | b      |
-            |         | list            | option b | c      |
-            | settings |                |          |        |
-            |          | id_string    | allow_choice_duplicates   |
-            |          | Duplicates   | Yes                       |
+            | survey  |                 |          |          |
+            |         | type            | name     | label    |
+            |         | select_one list | S1       | s1       |
+            | choices |                 |          |          |
+            |         | list name       | name     | label    |
+            |         | list            | a        | option a |
+            |         | list            | b        | option b |
+            |         | list            | b        | option c |
+            | settings |                |          |          |
+            |          | id_string      | allow_choice_duplicates   |
+            |          | Duplicates     | Yes                       |
             """
 
         expected = """
     <select1 ref="/pyxform_autotestname/S1">
       <label>s1</label>
       <item>
-        <label>a</label>
-        <value>option a</value>
+        <label>option a</label>
+        <value>a</value>
       </item>
       <item>
-        <label>b</label>
-        <value>option b</value>
+        <label>option b</label>
+        <value>b</value>
       </item>
       <item>
-        <label>c</label>
-        <value>option b</value>
+        <label>option c</label>
+        <value>b</value>
       </item>
     </select1>
 """
-        self.assertPyxformXform(md=md, xml__contains=[expected], run_odk_validate=True)
+        self.assertPyxformXform(md=md, xml__contains=[expected])
 
     def test_choice_list_without_duplicates_is_successful(self):
         md = """
@@ -145,4 +161,4 @@ class FieldsTests(PyxformTestCase):
       </item>
     </select1>
 """
-        self.assertPyxformXform(md=md, xml__contains=[expected], run_odk_validate=True)
+        self.assertPyxformXform(md=md, xml__contains=[expected])