Add warnings for convert_default, handle edge case (#94)

clize/converters.py

@@ -1,10 +1,11 @@
 # clize -- A command-line argument parser for Python
 # Copyright (C) 2011-2016 by Yann Kaiser and contributors. See AUTHORS and
 # COPYING for details.
-
+import contextlib
 import sys
 import io
 import os
+import warnings
 from functools import partial
 
 from sigtools.modifiers import autokwoargs
@@ -67,51 +68,53 @@ def __exit__(self, *exc_info):
         if self.arg != self.stdio or not self.keep_stdio_open:
             self.f.close()
 
-def _none_guard(cls, maybe_none, *args, **kwargs):
-    if maybe_none is None:
-        return None
-    else:
-        return cls(maybe_none, *args, **kwargs)
 
+@contextlib.contextmanager
+def _silence_convert_default_warning():
+    with warnings.catch_warnings():
+        warnings.filterwarnings("ignore", "The convert_default parameter of value_converter", DeprecationWarning, r"clize\..*")
+        yield
 
-@parser.value_converter(name='FILE', convert_default=True)
-@autokwoargs(exceptions=['arg'])
-def file(arg=util.UNSET, stdio='-', keep_stdio_open=False, **kwargs):
-    """Takes a file argument and provides a Python object that opens a file
 
-    ::
+with _silence_convert_default_warning():
+    @parser.value_converter(name='FILE', convert_default=True)
+    @autokwoargs(exceptions=['arg'])
+    def file(arg=util.UNSET, stdio='-', keep_stdio_open=False, **kwargs):
+        """Takes a file argument and provides a Python object that opens a file
 
-        def main(in_: file(), out: file(mode='w')):
-            with in_ as infile, out as outfile:
-                outfile.write(infile.read())
+        ::
 
-    :param stdio: If this value is passed as argument, it will be interpreted
-        as *stdin* or *stdout* depending on the ``mode`` parameter supplied.
-    :param keep_stdio_open: If true, does not close the file if it is *stdin*
-        or *stdout*.
+            def main(in_: file(), out: file(mode='w')):
+                with in_ as infile, out as outfile:
+                    outfile.write(infile.read())
 
-    Other arguments will be relayed to `io.open`.
+        :param stdio: If this value is passed as argument, it will be interpreted
+            as *stdin* or *stdout* depending on the ``mode`` parameter supplied.
+        :param keep_stdio_open: If true, does not close the file if it is *stdin*
+            or *stdout*.
 
-    This converter also opens the file or stream designated by the default
-    value::
+        Other arguments will be relayed to `io.open`.
 
-        def main(inf: file()='-'):
-            with inf as f:
-                print(f)
+        You can specify a default file name using `clize.Parameter.cli_default`::
 
-    .. code-block:: console
+            def main(inf: (file(), Parameter.cli_default("-"))):
+                with inf as f:
+                    print(f)
 
-        $ python3 ./main.py
-        <_io.TextIOWrapper name='<stdin>' mode='r' encoding='UTF-8'>
+        .. code-block:: console
 
+            $ python3 ./main.py
+            <_io.TextIOWrapper name='<stdin>' mode='r' encoding='UTF-8'>
 
-    """
-    if arg is not util.UNSET:
-        return _none_guard(_FileOpener, arg, kwargs, stdio, keep_stdio_open)
-    return parser.value_converter(
-        partial(_none_guard, _FileOpener, kwargs=kwargs,
-                stdio=stdio, keep_stdio_open=keep_stdio_open),
-        name='FILE', convert_default=True)
+
+        """
+        if arg is not util.UNSET:
+            return _FileOpener(arg, kwargs, stdio, keep_stdio_open)
+        with _silence_convert_default_warning():
+            return parser.value_converter(
+                partial(_FileOpener, kwargs=kwargs,
+                        stdio=stdio, keep_stdio_open=keep_stdio_open),
+                name='FILE', convert_default=True)
 
 
 def _convert_ioerror(arg, exc):

clize/parser.py

@@ -232,8 +232,7 @@ def __init__(self, **kwargs):
             display_name='<internal>', **kwargs)
 
 
-@modifiers.kwoargs(start='name')
-def value_converter(func=None, name=None, convert_default=False):
+def value_converter(func=None, *, name=None, convert_default=None, convert_default_filter=lambda s: isinstance(s, str)):
     """Callables decorated with this can be used as a value converter.
 
     :param str name: Use this name to designate the parameter value type.
@@ -244,18 +243,31 @@ def value_converter(func=None, name=None, convert_default=False):
         The default is the name of the decorated function or type, modified to
         follow this rule.
 
-    :param bool convert_default: If true, the value converter will be called
+    :param bool convert_default: *Deprecated*: use the `Parameter.cli_default()` annotation instead.
+
+        If true, the value converter will be called
         with the default parameter value if none was supplied. Otherwise, the
         default parameter is used as-is.
 
         Make sure to handle `None` appropriately if you override this.
 
+    :param function convert_convert_default_filter: *Deprecated* Avoid ``convert_default`` completely.
+
+        If ``convert_default`` is true, controls when the converter is called.
+        The converter is used only if the given function returns true.
+
     See :ref:`value converter`.
     """
+    if convert_default is not None:
+        warnings.warn("The convert_default parameter of value_converter is deprecated.  "
+                      "Direct users to use clize.Parameter.cli_default() instead.",
+                      DeprecationWarning,
+                      stacklevel=2)
     def decorate(func):
         info = {
             'name': util.name_type2cli(func) if name is None else name,
             'convert_default': convert_default,
+            'convert_default_filter': convert_default_filter,
         }
         try:
             func._clize__value_converter = info
@@ -390,19 +402,30 @@ def help_parens(self):
 
     def post_parse(self, ba):
         super(ParameterWithValue, self).post_parse(ba)
-        try:
-            info = self.conv._clize__value_converter
-        except AttributeError:
-            pass
-        else:
-            if self in ba.not_provided:
-                if self.cli_default is not util.UNSET:
-                    self.set_value(
-                        ba,
-                        self.cli_default.value_after_conversion(partial(self.coerce_value, ba=ba))
-                    )
-                elif self.default is not util.UNSET and info['convert_default']:
-                    self.set_value(ba, self.coerce_value(self.default, ba))
+        if self in ba.not_provided:
+            default_value = self.default_value_if_non_source_default(ba)
+            if default_value is not util.UNSET:
+                self.set_value(ba, default_value)
+
+    def default_value_if_non_source_default(self, ba):
+        if self.cli_default is not util.UNSET:
+            return self.cli_default.value_after_conversion(partial(self.coerce_value, ba=ba))
+        if self.default is not util.UNSET:
+            try:
+                info = self.conv._clize__value_converter
+            except AttributeError:
+                pass
+            else:
+                if info['convert_default'] and info['convert_default_filter'](self.default):
+                    return self.conv(self.default)
+        return util.UNSET
+
+    def default_value(self, ba):
+        converted_default = self.default_value_if_non_source_default(ba)
+        if converted_default is not util.UNSET:
+            return converted_default
+        return self.default
+
 
 
 class NamedParameter(Parameter):
@@ -617,10 +640,10 @@ def set_value(self, ba, val):
                 return
             else:
                 if arg is util.UNSET:
-                    if param.cli_default != util.UNSET:
-                        ba.args.append(param.cli_default.value_after_conversion(partial(self.coerce_value, ba=ba)))
-                    elif param.default != util.UNSET:
-                        ba.args.append(param.default)
+                    default_value = param.default_value(ba)
+                    if default_value is not util.UNSET:
+                        ba.args.append(default_value)
+                        # ba.args.append(param.cli_default.value_after_conversion(partial(self.coerce_value, ba=ba)))
                     else:
                         raise ValueError(
                             "Can't set parameters after required parameters")
@@ -967,6 +990,24 @@ def _use_class(pos_cls, varargs_cls, named_cls, varkwargs_cls, kwargs,
             "with clize.parser.value_converter()"
         )
 
+    if kwargs['default'] is not util.UNSET:
+        try:
+            info = getattr(kwargs['conv'], "_clize__value_converter")
+        except AttributeError:
+            pass
+        else:
+            if info["convert_default"] and info["convert_default_filter"](kwargs["default"]):
+                warnings.warn(
+                    f"For parameter '{param.name}':  "
+                    "Default argument conversion is deprecated.  "
+                    "Instead, please use clize.Parameter.cli_default(value) "
+                    "when you need a default value that "
+                    "is converted the same way as a value passed in from the command line, "
+                    "as opposed to values passed in from calling the function in Python, "
+                    "which aren't converted by Clize.",
+                    DeprecationWarning
+                )
+
     if named:
         kwargs['aliases'] = aliases
         if param.kind == param.VAR_KEYWORD:

clize/runner.py

@@ -1,10 +1,12 @@
 # clize -- A command-line argument parser for Python
 # Copyright (C) 2011-2016 by Yann Kaiser and contributors. See AUTHORS and
 # COPYING for details.
+import contextlib
 import pathlib
 import sys
 import os
 import typing
+import warnings
 from functools import partial, update_wrapper
 import itertools
 import shutil
@@ -213,14 +215,33 @@ def helper(self):
     @util.property_once
     def signature(self):
         """The `.parser.CliSignature` object used to parse arguments."""
-        return parser.CliSignature.from_signature(
-            self.func_signature,
-            extra=itertools.chain(self._process_alt(self.alt), self.extra))
+        extra = itertools.chain(self._process_alt(self.alt), self.extra)
+        with self._move_warnings_to_func():
+            return parser.CliSignature.from_signature(
+                self.func_signature,
+                extra=extra)
 
     @util.property_once
     def func_signature(self):
         return signature(self.func)
 
+    @contextlib.contextmanager
+    def _move_warnings_to_func(self):
+        try:
+            code = self.func.__code__
+            filename = code.co_filename
+            lineno = code.co_firstlineno
+            module = self.func.__module__
+            module_globals = self.func.__globals__
+        except AttributeError:
+            yield
+        else:
+            with warnings.catch_warnings(record=True) as caught_warnings:
+                yield
+            for warning in caught_warnings:
+                registry = module_globals.setdefault("__warningregistry__", {})
+                warnings.warn_explicit(warning.message, warning.category, filename, lineno, module, registry, module_globals)
+
     def _process_alt(self, alt):
         if self.help_names:
             p = parser.FallbackCommandParameter(

clize/tests/test_converters.py

@@ -12,7 +12,7 @@
 
 from sigtools import support, modifiers
 
-from clize import parser, errors, converters
+from clize import parser, errors, converters, Parameter
 from clize.tests.util import Fixtures, SignatureFixtures, Tests
 
 
@@ -149,11 +149,25 @@ def func(afile):
             self.assertFalse(stderr.getvalue())
         self.assertTrue(self.completed)
 
-    def test_default_value(self):
+    def test_deprecated_default_value(self):
         path = os.path.join(self.temp, 'default')
         open(path, 'w').close()
-        @modifiers.annotate(afile=converters.file())
-        def func(afile=path):
+        def func(afile: converters.file()=path):
+            with afile as f:
+                self.assertEqual(f.name, path)
+                self.assertEqual(f.mode, 'r')
+            self.assertTrue(f.closed)
+            self.completed = True
+        with self.assertWarns(DeprecationWarning):
+            stdout, stderr = self.crun(func, ['test'])
+        self.assertFalse(stdout.getvalue())
+        self.assertFalse(stderr.getvalue())
+        self.assertTrue(self.completed)
+
+    def test_cli_default_value(self):
+        path = os.path.join(self.temp, 'default')
+        open(path, 'w').close()
+        def func(afile: (converters.file(), Parameter.cli_default(path))):
             with afile as f:
                 self.assertEqual(f.name, path)
                 self.assertEqual(f.mode, 'r')
@@ -165,8 +179,7 @@ def func(afile=path):
         self.assertTrue(self.completed)
 
     def test_default_none_value(self):
-        @modifiers.annotate(afile=converters.file())
-        def func(afile=None):
+        def func(afile: converters.file() = None):
             self.assertIs(afile, None)
             self.completed = True
         stdout, stderr = self.crun(func, ['test'])