Re-harmonize argument parsing between distutils and optparse CLIs

babel/messages/frontend.py

@@ -22,7 +22,7 @@
 
 from babel import __version__ as VERSION
 from babel import Locale, localedata
-from babel._compat import StringIO, string_types
+from babel._compat import StringIO, string_types, text_type
 from babel.core import UnknownLocaleError
 from babel.messages.catalog import Catalog
 from babel.messages.extract import DEFAULT_KEYWORDS, DEFAULT_MAPPING, check_and_call_extract_file, extract_from_dir
@@ -39,6 +39,48 @@
     from configparser import RawConfigParser
 
 
+def listify_value(arg, split=None):
+    """
+    Make a list out of an argument.
+
+    Values from `distutils` argument parsing are always single strings;
+    values from `optparse` parsing may be lists of strings that may need
+    to be further split.
+
+    No matter the input, this function returns a flat list of whitespace-trimmed
+    strings, with `None` values filtered out.
+
+    >>> listify_value("foo bar")
+    ['foo', 'bar']
+    >>> listify_value(["foo bar"])
+    ['foo', 'bar']
+    >>> listify_value([["foo"], "bar"])
+    ['foo', 'bar']
+    >>> listify_value([["foo"], ["bar", None, "foo"]])
+    ['foo', 'bar', 'foo']
+    >>> listify_value("foo, bar, quux", ",")
+    ['foo', 'bar', 'quux']
+
+    :param arg: A string or a list of strings
+    :param split: The argument to pass to `str.split()`.
+    :return:
+    """
+    out = []
+
+    if not isinstance(arg, (list, tuple)):
+        arg = [arg]
+
+    for val in arg:
+        if val is None:
+            continue
+        if isinstance(val, (list, tuple)):
+            out.extend(listify_value(val, split=split))
+            continue
+        out.extend(s.strip() for s in text_type(val).split(split))
+    assert all(isinstance(val, string_types) for val in out)
+    return out
+
+
 class Command(_Command):
     # This class is a small shim between Distutils commands and
     # optparse option parsing in the frontend command line.
@@ -47,8 +89,21 @@ class Command(_Command):
     as_args = None
 
     #: Options which allow multiple values.
+    #: This is used by the `optparse` transmogrification code.
     multiple_value_options = ()
 
+    #: Options which are booleans.
+    #: This is used by the `optparse` transmogrification code.
+    # (This is actually used by distutils code too, but is never
+    # declared in the base class.)
+    boolean_options = ()
+
+    #: Option aliases, to retain standalone command compatibility.
+    #: Distutils does not support option aliases, but optparse does.
+    #: This maps the distutils argument name to an iterable of aliases
+    #: that are usable with optparse.
+    option_aliases = {}
+
     #: Log object. To allow replacement in the script command line runner.
     log = distutils_log
 
@@ -110,6 +165,7 @@ def initialize_options(self):
         self.statistics = False
 
     def finalize_options(self):
+        self.domain = listify_value(self.domain)
         if not self.input_file and not self.directory:
             raise DistutilsOptionError('you must specify either the input file '
                                        'or the base directory')
@@ -118,9 +174,7 @@ def finalize_options(self):
                                        'or the base directory')
 
     def run(self):
-        domains = self.domain.split()
-
-        for domain in domains:
+        for domain in self.domain:
             self._run_domain(domain)
 
     def _run_domain(self, domain):
@@ -174,12 +228,12 @@ def _run_domain(self, domain):
                 if len(catalog):
                     percentage = translated * 100 // len(catalog)
                 self.log.info(
-                    '%d of %d messages (%d%%) translated in %r',
+                    '%d of %d messages (%d%%) translated in %s',
                     translated, len(catalog), percentage, po_file
                 )
 
             if catalog.fuzzy and not self.use_fuzzy:
-                self.log.info('catalog %r is marked as fuzzy, skipping', po_file)
+                self.log.info('catalog %s is marked as fuzzy, skipping', po_file)
                 continue
 
             for message, errors in catalog.check():
@@ -188,7 +242,7 @@ def _run_domain(self, domain):
                         'error: %s:%d: %s', po_file, message.lineno, error
                     )
 
-            self.log.info('compiling catalog %r to %r', po_file, mo_file)
+            self.log.info('compiling catalog %s to %s', po_file, mo_file)
 
             outfile = open(mo_file, 'wb')
             try:
@@ -249,7 +303,7 @@ class extract_messages(Command):
         ('add-comments=', 'c',
          'place comment block with TAG (or those preceding keyword lines) in '
          'output file. Separate multiple TAGs with commas(,)'),  # TODO: Support repetition of this argument
-        ('strip-comments', None,
+        ('strip-comments', 's',
          'strip the comment TAGs from the comments.'),
         ('input-paths=', None,
          'files or directories that should be scanned for messages. Separate multiple '
@@ -263,6 +317,12 @@ class extract_messages(Command):
     ]
     as_args = 'input-paths'
     multiple_value_options = ('add-comments', 'keywords')
+    option_aliases = {
+        'keywords': ('--keyword',),
+        'mapping-file': ('--mapping',),
+        'output-file': ('--output',),
+        'strip-comments': ('--strip-comment-tags',),
+    }
 
     def initialize_options(self):
         self.charset = 'utf-8'
@@ -299,8 +359,7 @@ def finalize_options(self):
         else:
             keywords = DEFAULT_KEYWORDS.copy()
 
-        for kwarg in (self.keywords or ()):
-            keywords.update(parse_keywords(kwarg.split()))
+        keywords.update(parse_keywords(listify_value(self.keywords)))
 
         self.keywords = keywords
 
@@ -325,11 +384,13 @@ def finalize_options(self):
         if self.input_paths:
             if isinstance(self.input_paths, string_types):
                 self.input_paths = re.split(',\s*', self.input_paths)
-        else:
+        elif self.distribution is not None:
             self.input_paths = dict.fromkeys([
                 k.split('.', 1)[0]
                 for k in (self.distribution.packages or ())
             ]).keys()
+        else:
+            self.input_paths = []
 
         if not self.input_paths:
             raise DistutilsOptionError("no input files or directories specified")
@@ -338,11 +399,7 @@ def finalize_options(self):
             if not os.path.exists(path):
                 raise DistutilsOptionError("Input path: %s does not exist" % path)
 
-        if self.add_comments:
-            if isinstance(self.add_comments, string_types):
-                self.add_comments = self.add_comments.split(',')
-        else:
-            self.add_comments = []
+        self.add_comments = listify_value(self.add_comments or (), ",")
 
         if self.distribution:
             if not self.project:
@@ -531,7 +588,7 @@ def finalize_options(self):
 
     def run(self):
         self.log.info(
-            'creating catalog %r based on %r', self.output_file, self.input_file
+            'creating catalog %s based on %s', self.output_file, self.input_file
         )
 
         infile = open(self.input_file, 'rb')
@@ -662,7 +719,7 @@ def run(self):
             raise DistutilsOptionError('no message catalogs found')
 
         for locale, filename in po_files:
-            self.log.info('updating catalog %r based on %r', filename, self.input_file)
+            self.log.info('updating catalog %s based on %s', filename, self.input_file)
             infile = open(filename, 'rb')
             try:
                 catalog = read_po(infile, locale=locale, domain=domain)
@@ -825,6 +882,7 @@ def _configure_command(self, cmdname, argv):
             strs = ["--%s" % name]
             if short:
                 strs.append("-%s" % short)
+            strs.extend(cmdclass.option_aliases.get(name, ()))
             if name == as_args:
                 parser.usage += "<%s>" % name
             elif name in cmdclass.boolean_options:

tests/messages/test_frontend.py

@@ -1092,7 +1092,7 @@ def test_compile_catalog(self):
                                  '-d', self._i18n_dir()])
         assert not os.path.isfile(mo_file), 'Expected no file at %r' % mo_file
         self.assertEqual("""\
-catalog %r is marked as fuzzy, skipping
+catalog %s is marked as fuzzy, skipping
 """ % (po_file), sys.stderr.getvalue())
 
     def test_compile_fuzzy_catalog(self):
@@ -1104,7 +1104,7 @@ def test_compile_fuzzy_catalog(self):
                                      '-d', self._i18n_dir()])
             assert os.path.isfile(mo_file)
             self.assertEqual("""\
-compiling catalog %r to %r
+compiling catalog %s to %s
 """ % (po_file, mo_file), sys.stderr.getvalue())
         finally:
             if os.path.isfile(mo_file):
@@ -1123,7 +1123,7 @@ def test_compile_catalog_with_more_than_2_plural_forms(self):
                                      '-d', self._i18n_dir()])
             assert os.path.isfile(mo_file)
             self.assertEqual("""\
-compiling catalog %r to %r
+compiling catalog %s to %s
 """ % (po_file, mo_file), sys.stderr.getvalue())
         finally:
             if os.path.isfile(mo_file):
@@ -1143,8 +1143,8 @@ def test_compile_catalog_multidomain(self):
             for mo_file in [mo_foo, mo_bar]:
                 assert os.path.isfile(mo_file)
             self.assertEqual("""\
-compiling catalog %r to %r
-compiling catalog %r to %r
+compiling catalog %s to %s
+compiling catalog %s to %s
 """ % (po_foo, mo_foo, po_bar, mo_bar), sys.stderr.getvalue())
 
         finally:
@@ -1245,9 +1245,34 @@ def configure_cli_command(cmdline):
     return cmdinst
 
 
+def configure_distutils_command(cmdline):
+    """
+    Helper to configure a command class, but not run it just yet.
+
+    This will have strange side effects if you pass in things
+    `distutils` deals with internally.
+
+    :param cmdline: The command line (sans the executable name)
+    :return: Command instance
+    """
+    d = Distribution(attrs={
+        "cmdclass": vars(frontend),
+        "script_args": shlex.split(cmdline),
+    })
+    d.parse_command_line()
+    assert len(d.commands) == 1
+    cmdinst = d.get_command_obj(d.commands[0])
+    cmdinst.ensure_finalized()
+    return cmdinst
+
+
 @pytest.mark.parametrize("split", (False, True))
-def test_extract_keyword_args_384(split):
+@pytest.mark.parametrize("arg_name", ("-k", "--keyword", "--keywords"))
+def test_extract_keyword_args_384(split, arg_name):
     # This is a regression test for https://github.com/python-babel/babel/issues/384
+    # and it also tests that the rest of the forgotten aliases/shorthands implied by
+    # https://github.com/python-babel/babel/issues/390 are re-remembered (or rather
+    # that the mechanism for remembering them again works).
 
     kwarg_specs = [
         "gettext_noop",
@@ -1261,9 +1286,9 @@ def test_extract_keyword_args_384(split):
     ]
 
     if split:  # Generate a command line with multiple -ks
-        kwarg_text = " ".join("-k %s" % kwarg_spec for kwarg_spec in kwarg_specs)
+        kwarg_text = " ".join("%s %s" % (arg_name, kwarg_spec) for kwarg_spec in kwarg_specs)
     else:  # Generate a single space-separated -k
-        kwarg_text = "-k \"%s\"" % " ".join(kwarg_specs)
+        kwarg_text = "%s \"%s\"" % (arg_name, " ".join(kwarg_specs))
 
     # (Both of those invocation styles should be equivalent, so there is no parametrization from here on out)
 
@@ -1293,10 +1318,39 @@ def test_extract_keyword_args_384(split):
     ))
 
 
+@pytest.mark.parametrize("kwarg,expected", [
+    ("LW_", ("LW_",)),
+    ("LW_ QQ Q", ("LW_", "QQ", "Q")),
+    ("yiy         aia", ("yiy", "aia")),
+])
+def test_extract_distutils_keyword_arg_388(kwarg, expected):
+    # This is a regression test for https://github.com/python-babel/babel/issues/388
+
+    # Note that distutils-based commands only support a single repetition of the same argument;
+    # hence `--keyword ignored` will actually never end up in the output.
+
+    cmdinst = configure_distutils_command(
+        "extract_messages --no-default-keywords --keyword ignored --keyword '%s' "
+        "--input-dirs . --output-file django233.pot --add-comments Bar,Foo" % kwarg
+    )
+    assert isinstance(cmdinst, extract_messages)
+    assert set(cmdinst.keywords.keys()) == set(expected)
+
+    # Test the comma-separated comment argument while we're at it:
+    assert set(cmdinst.add_comments) == set(("Bar", "Foo"))
+
+
 def test_update_catalog_boolean_args():
     cmdinst = configure_cli_command("update --no-wrap -N --ignore-obsolete --previous -i foo -o foo -l en")
     assert isinstance(cmdinst, update_catalog)
     assert cmdinst.no_wrap is True
     assert cmdinst.no_fuzzy_matching is True
     assert cmdinst.ignore_obsolete is True
     assert cmdinst.previous is False  # Mutually exclusive with no_fuzzy_matching
+
+
+def test_extract_cli_knows_dash_s():
+    # This is a regression test for https://github.com/python-babel/babel/issues/390
+    cmdinst = configure_cli_command("extract -s -o foo babel")
+    assert isinstance(cmdinst, extract_messages)
+    assert cmdinst.strip_comments