Merge pull request #725 from lsst/tickets/DM-35917

DM-35917: Update split_commas to understand [] list syntax

doc/changes/DM-35917.misc.rst

@@ -0,0 +1 @@
+For command-line options that split on commas, it is now possible to specify parts of the string not to split by using ``[]`` to indicate comma-separated list content.

python/lsst/daf/butler/cli/opt/options.py

@@ -113,10 +113,15 @@ def makeCollectionTypes(context, param, value):
 )
 
 
+def _config_split(*args):
+    # Config values might include commas so disable comma-splitting.
+    return split_kv(*args, multiple=False)
+
+
 config_option = MWOptionDecorator(
     "-c",
     "--config",
-    callback=split_kv,
+    callback=_config_split,
     help="Config override, as a key-value pair.",
     metavar="TEXT=TEXT",
     multiple=True,

python/lsst/daf/butler/cli/utils.py

@@ -52,10 +52,12 @@
 import itertools
 import logging
 import os
+import re
 import sys
 import textwrap
 import traceback
 import uuid
+import warnings
 from collections import Counter
 from contextlib import contextmanager
 from functools import partial, wraps
@@ -240,7 +242,8 @@ def split_commas(context, param, values):
     values, and return a single list of all the passed-in values.
 
     This function can be passed to the 'callback' argument of a click.option to
-    allow it to process comma-separated values (e.g. "--my-opt a,b,c").
+    allow it to process comma-separated values (e.g. "--my-opt a,b,c"). If
+    the comma is inside ``[]`` there will be no splitting.
 
     Parameters
     ----------
@@ -250,21 +253,62 @@ def split_commas(context, param, values):
     param : `click.core.Option` or `None`
         The parameter being handled. Unused, but Click always passes it to
         callbacks.
-    values : [`str`]
+    values : iterable of `str` or `str`
         All the values passed for this option. Strings may contain commas,
-        which will be treated as delimiters for separate values.
+        which will be treated as delimiters for separate values unless they
+        are within ``[]``.
 
     Returns
     -------
-    list of string
-        The passed in values separated by commas and combined into a single
-        list.
+    results : `tuple` [`str`]
+        The passed in values separated by commas where appropriate and
+        combined into a single tuple.
     """
     if values is None:
         return values
     valueList = []
     for value in ensure_iterable(values):
-        valueList.extend(value.split(","))
+        # If we have [, or ,] we do the slow split. If square brackets
+        # are not matching then that is likely a typo that should result
+        # in a warning.
+        opens = "["
+        closes = "]"
+        if re.search(rf"\{opens}.*,|,.*\{closes}", value):
+            in_parens = False
+            current = ""
+            for c in value:
+                if c == opens:
+                    if in_parens:
+                        warnings.warn(
+                            f"Found second opening {opens} without corresponding closing {closes}"
+                            f" in {value!r}",
+                            stacklevel=2,
+                        )
+                    in_parens = True
+                elif c == closes:
+                    if not in_parens:
+                        warnings.warn(
+                            f"Found second closing {closes} without corresponding open {opens} in {value!r}",
+                            stacklevel=2,
+                        )
+                    in_parens = False
+                elif c == ",":
+                    if not in_parens:
+                        # Split on this comma.
+                        valueList.append(current)
+                        current = ""
+                        continue
+                current += c
+            if in_parens:
+                warnings.warn(
+                    f"Found opening {opens} that was never closed in {value!r}",
+                    stacklevel=2,
+                )
+            if current:
+                valueList.append(current)
+        else:
+            # Use efficient split since no parens.
+            valueList.extend(value.split(","))
     return tuple(valueList)
 
 
@@ -420,10 +464,10 @@ def get(self):
                 k, v = val.split(separator)
                 if choice is not None:
                     choice(v)  # will raise if val is an invalid choice
-            except ValueError:
+            except ValueError as e:
                 raise click.ClickException(
                     message=f"Could not parse key-value pair '{val}' using separator '{separator}', "
-                    f"with multiple values {'allowed' if multiple else 'not allowed'}."
+                    f"with multiple values {'allowed' if multiple else 'not allowed'}: {e}"
                 )
             ret.add(k, norm(v))
     return ret.get()

python/lsst/daf/butler/registry/tests/_registry.py

@@ -2989,12 +2989,12 @@ def testDatasetIdFactory(self):
 
         datasetId = factory.makeDatasetId(run, dataset_type, data_id, DatasetIdGenEnum.UNIQUE)
         self.assertIsInstance(datasetId, uuid.UUID)
-        self.assertEquals(datasetId.version, 4)
+        self.assertEqual(datasetId.version, 4)
 
         datasetId = factory.makeDatasetId(run, dataset_type, data_id, DatasetIdGenEnum.DATAID_TYPE)
         self.assertIsInstance(datasetId, uuid.UUID)
-        self.assertEquals(datasetId.version, 5)
+        self.assertEqual(datasetId.version, 5)
 
         datasetId = factory.makeDatasetId(run, dataset_type, data_id, DatasetIdGenEnum.DATAID_TYPE_RUN)
         self.assertIsInstance(datasetId, uuid.UUID)
-        self.assertEquals(datasetId.version, 5)
+        self.assertEqual(datasetId.version, 5)

tests/test_cliUtilSplitCommas.py

@@ -84,6 +84,28 @@ def test_none(self):
         self.assertEqual(result.exit_code, 0, msg=clickResultMsg(result))
         mock.assert_called_with(())
 
+    def test_parens(self):
+        """Test that split commas understands [a,b]"""
+        for test, expected in (
+            ("single", ("single",)),
+            ("a,b", ("a", "b")),
+            ("[a,b]", ("[a,b]",)),
+            ("a[1,2],b", ("a[1,2]", "b")),
+        ):
+            result = split_commas(None, None, test)
+            self.assertEqual(result, expected)
+
+        # These should warn because it's likely a typo.
+        for test, expected in (
+            ("a[1,b[2,3],c", ("a[1,b[2,3]", "c")),
+            ("a[1,b,c", ("a[1,b,c",)),
+            ("a[1,b", ("a[1,b",)),
+            ("a1,b]", ("a1", "b]")),
+        ):
+            with self.assertWarns(UserWarning, msg=f"Testing {test!r}"):
+                result = split_commas(None, None, test)
+            self.assertEqual(result, expected)
+
 
 if __name__ == "__main__":
     unittest.main()

tests/test_cliUtilSplitKv.py

@@ -166,7 +166,7 @@ def cli(value):
         self.assertEqual(
             result.output,
             "Error: Could not parse key-value pair 'first==1' using separator '=', with "
-            "multiple values allowed.\n",
+            "multiple values allowed: too many values to unpack (expected 2)\n",
         )
 
     def test_choice(self):