From bffbefae454fbe56e9975f180d9be5b6f9917ef5 Mon Sep 17 00:00:00 2001 From: Yann Kaiser Date: Sat, 8 Oct 2022 19:25:57 -0700 Subject: [PATCH] Add warnings for convert_default, handle edge case (#94) --- clize/converters.py | 71 +++++++++++++++-------------- clize/parser.py | 81 +++++++++++++++++++++++++--------- clize/runner.py | 27 ++++++++++-- clize/tests/test_converters.py | 25 ++++++++--- clize/tests/test_parser.py | 51 ++++++++++++--------- clize/tests/util.py | 10 +++++ 6 files changed, 182 insertions(+), 83 deletions(-) diff --git a/clize/converters.py b/clize/converters.py index bf94377..b3e3ae7 100644 --- a/clize/converters.py +++ b/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='' mode='r' encoding='UTF-8'> + .. code-block:: console + $ python3 ./main.py + <_io.TextIOWrapper name='' 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): diff --git a/clize/parser.py b/clize/parser.py index ce73950..ae207d6 100644 --- a/clize/parser.py +++ b/clize/parser.py @@ -232,8 +232,7 @@ def __init__(self, **kwargs): display_name='', **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: diff --git a/clize/runner.py b/clize/runner.py index d900875..3d4ae8f 100644 --- a/clize/runner.py +++ b/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( diff --git a/clize/tests/test_converters.py b/clize/tests/test_converters.py index f282973..045e088 100644 --- a/clize/tests/test_converters.py +++ b/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']) diff --git a/clize/tests/test_parser.py b/clize/tests/test_parser.py index 660dc24..df4a64b 100644 --- a/clize/tests/test_parser.py +++ b/clize/tests/test_parser.py @@ -9,7 +9,7 @@ import warnings import repeated_test -from repeated_test import evaluated +from repeated_test import evaluated, options from sigtools import support, modifiers, specifiers from clize import parser, errors, util, Clize, Parameter @@ -360,9 +360,11 @@ def conv_not_default(arg): class SigTests(SignatureFixtures): - def _test(self, sig, str_rep, args, posargs, kwargs, *, make_signature): - csig = parser.CliSignature.from_signature(sig) - ba = self.read_arguments(csig, args) + def _test(self, sig, str_rep, args, posargs, kwargs, *, make_signature, conversion_warning=None, parsing_warning=None): + with self.maybe_expect_warning(conversion_warning): + csig = parser.CliSignature.from_signature(sig) + with self.maybe_expect_warning(parsing_warning): + ba = self.read_arguments(csig, args) self.assertEqual(str(csig), str_rep) self.assertEqual(ba.args, posargs) self.assertEqual(ba.kwargs, kwargs) @@ -560,35 +562,39 @@ def conv(arg): @evaluated def vconverter_convert_value_equals(self, *, make_signature): - @parser.value_converter(convert_default=True) - def conv(arg): - return 'c{}c'.format(arg) + with self.assertWarns(DeprecationWarning): + @parser.value_converter(convert_default=True) + def conv(arg): + return 'c{}c'.format(arg) sig = make_signature('*, par:conv="default"', globals={'conv': conv}) - return (sig, '[--par=CONV]', ('--par=A',), [], {'par': 'cAc'}) + return (sig, '[--par=CONV]', ('--par=A',), [], {'par': 'cAc'}, options(conversion_warning=DeprecationWarning)) @evaluated def vconverter_convert_value_spaced(self, *, make_signature): - @parser.value_converter(convert_default=True) - def conv(arg): - return 'c{}c'.format(arg) + with self.assertWarns(DeprecationWarning): + @parser.value_converter(convert_default=True) + def conv(arg): + return 'c{}c'.format(arg) sig = make_signature('*, par:conv="default"', globals={'conv': conv}) - return (sig, '[--par=CONV]', ('--par', 'A',), [], {'par': 'cAc'}) + return (sig, '[--par=CONV]', ('--par', 'A',), [], {'par': 'cAc'}, options(conversion_warning=DeprecationWarning)) @evaluated def vconverter_convert_default(self, *, make_signature): - @parser.value_converter(convert_default=True) - def conv(arg): - return 'converted' + with self.assertWarns(DeprecationWarning): + @parser.value_converter(convert_default=True) + def conv(arg): + return 'converted' sig = make_signature('*, par:conv="default"', globals={'conv': conv}) - return (sig, '[--par=CONV]', (), [], {'par': 'converted'}) + return (sig, '[--par=CONV]', (), [], {'par': 'converted'}, options(conversion_warning=DeprecationWarning)) @evaluated def vconverter_convert_default_after_pos(self, *, make_signature): - @parser.value_converter(convert_default=True) - def conv(arg): - return 'converted' + with self.assertWarns(DeprecationWarning): + @parser.value_converter(convert_default=True) + def conv(arg): + return 'converted' sig = make_signature('first="otherdefault", par:conv="default"', globals={'conv': conv}) - return (sig, '[first] [par]', (), ['otherdefault', 'converted'], {}) + return (sig, '[first] [par]', (), ['otherdefault', 'converted'], {}, options(conversion_warning=DeprecationWarning)) cli_default_no_src_default = (s( 'par: ann', @@ -614,6 +620,11 @@ def conv(arg): 'first="otherdefault", par:conv="src_default"', conv=Parameter.cli_default("default") ), "[first] [par]", (), ["otherdefault", "default"], {}) + cli_default_for_default_converter = (s( + 'par: ann', + ann=(int, Parameter.cli_default("1234")), + ), "[par]", (), [1234], {}) + class ExtraParamsTests(Fixtures): def _test(self, sig_str, extra, args, posargs, kwargs, func): diff --git a/clize/tests/util.py b/clize/tests/util.py index 845fb89..8ec94c2 100644 --- a/clize/tests/util.py +++ b/clize/tests/util.py @@ -51,6 +51,16 @@ def assertLinesEqual(self, expected, actual): (line.rstrip() for line in actual.split('\n')))) self.assertEqual(exp_split, act_split) + @contextmanager + def maybe_expect_warning(self, maybe_warning): + if maybe_warning is not None: + with self.assertWarns(maybe_warning) as warn_context: + yield warn_context + else: + yield None + + + support_s_without_annotations_feature = repeated_test.NamedAlternative("no __future__ features", support.s) @repeated_test.NamedAlternative("with __future__.annotations")