diff --git a/i2/deco.py b/i2/deco.py index ec85265b..32ab3828 100644 --- a/i2/deco.py +++ b/i2/deco.py @@ -210,17 +210,35 @@ def from_jdict(cls, jdict): return FuncFactory(**jdict) -def _double_up_as_factory(wrapped=None, *args, __decorator_func=None, **kwargs): - """Util for double_up_as_factory, ``__decorator_func`` to be partialized""" +def _double_up_as_factory( + wrapped=None, + *args, + __decorator_func=None, + __wrapped_param_name=None, + **kwargs, +): + """Util for double_up_as_factory; the ``__``-prefixed params are partialized in. + + ``__decorator_func`` is the decorator being doubled up, and + ``__wrapped_param_name`` is the name that decorator gave its first parameter -- + needed so that the object to wrap can be given by keyword as well as positionally. + """ if args: raise RuntimeError( f"You need to specify decorator arguments as keyword-only." f"You specified positional arguments: {args=}" ) + if wrapped is None: + # The object to wrap may have been given by keyword (``decorator(func=foo)``), + # in which case it landed in kwargs, under the decorator's first param name. + # Note we only look for it when nothing was given positionally: if it was given + # both ways, leaving kwargs alone lets python raise its own (clearer) + # "got multiple values for argument" TypeError. + wrapped = kwargs.pop(__wrapped_param_name, None) if wrapped is None: # then we want a factory return partial(__decorator_func, **kwargs) else: - return __decorator_func(wrapped, *args, **kwargs) + return __decorator_func(wrapped, **kwargs) def double_up_as_factory(decorator_func): @@ -246,7 +264,27 @@ def double_up_as_factory(decorator_func): >>> wrapped_foo = decorator(foo, multiplier=10) >>> wrapped_foo(2) 30 - >>> + + The object to wrap doesn't have to be given positionally: it can also be given by + keyword, under the name the decorator gave its first parameter (here, ``func``). + This matters because forwarding arguments through ``**kwargs`` is a very common way + to call a decorator, so ``decorator(func=foo)`` must mean what ``decorator(foo)`` + means: + + >>> decorator(func=foo, multiplier=10)(2) + 30 + >>> decorator(func=foo)(2) + 6 + + It is the *absence* of an object to wrap -- not the way it's passed -- that asks for + a factory: + + >>> from functools import partial + >>> isinstance(decorator(multiplier=3), partial) + True + >>> isinstance(decorator(func=foo), partial) + False + >>> multiply_by_3 = decorator(multiplier=3) >>> wrapped_foo = multiply_by_3(foo) >>> wrapped_foo(2) @@ -275,9 +313,38 @@ def double_up_as_factory(decorator_func): ... AssertionError: All arguments (besides the first) need to be keyword-only + Note also that the name of that first argument is effectively **reserved**: it always + means "the object to wrap". For a decorator that also takes ``**kwargs``, this means + a decorator argument can never share that name. Say a decorator's first parameter is + ``func`` and it renames parameters via ``**kwargs``: + + >>> @double_up_as_factory + ... def rename(func=None, **new_name_for_old_name): + ... return new_name_for_old_name # (stand-in for the real work) + + You can rename an ordinary parameter through the factory form: + + >>> rename(b='bee')(lambda a, b: None) + {'b': 'bee'} + + But you cannot use it to rename a parameter that happens to be called ``func``: + ``rename(func='callback')`` is read as "wrap the object ``'callback'``", not as + "rename ``func`` to ``callback``", so it returns nonsense rather than a factory: + + >>> rename(func='callback') + {} + + This is a pre-existing limitation of the double-up idiom -- there is no way to tell + the two intents apart -- and it is not specific to passing the object by keyword. + Before keyword-passing was supported the same call failed later and differently, + with ``TypeError: rename() got multiple values for argument 'func'``. If a decorator + needs an argument with the same name as its first parameter, don't use + ``double_up_as_factory``. + """ - def validate_decorator_func(decorator_func): + def validated_wrapped_param_name(decorator_func): + """Validate decorator_func, returning the name of its first parameter.""" first_param, *other_params = signature(decorator_func).parameters.values() assert first_param.default is None, ( f"First argument of the decorator function needs to default to None. " @@ -286,12 +353,14 @@ def validate_decorator_func(decorator_func): assert all( p.kind in {p.KEYWORD_ONLY, p.VAR_KEYWORD} for p in other_params ), f"All arguments (besides the first) need to be keyword-only" - return True - - validate_decorator_func(decorator_func) + return first_param.name return wraps(decorator_func)( - partial(_double_up_as_factory, __decorator_func=decorator_func) + partial( + _double_up_as_factory, + __decorator_func=decorator_func, + __wrapped_param_name=validated_wrapped_param_name(decorator_func), + ) ) diff --git a/i2/tests/test_wrapper.py b/i2/tests/test_wrapper.py index 07eba39c..9a5c9393 100644 --- a/i2/tests/test_wrapper.py +++ b/i2/tests/test_wrapper.py @@ -1,9 +1,20 @@ """Testing wrapper""" from collections.abc import Iterable -from i2.wrapper import wrap, mk_ingress_from_name_mapper, rm_params +from functools import partial + +import pytest + +from i2.wrapper import ( + wrap, + mk_ingress_from_name_mapper, + rm_params, + ch_names, + include_exclude, + add_smart_defaults, +) from i2.deco import FuncFactory -from i2.signatures import Sig +from i2.signatures import Sig, name_of_obj def _test_ingress(a, b: str, c="hi"): @@ -425,3 +436,101 @@ def egress(output): # No annotation assert sig.return_annotation is Parameter.empty # Test functionality assert wrapped(5) == 10 + + +# --------------------------------------------------------------------------------------- +# double_up_as_factory: the object to wrap can be given positionally OR by keyword +# See https://github.com/i2mint/i2/issues/64 + +#: The ``double_up_as_factory``-built decorators of ``i2.wrapper``. Each takes the object +#: to wrap as its first parameter (named ``func``) and every other parameter is +#: keyword-only with a default, so each must be usable in all three of these ways: +#: ``deco(func)``, ``deco(func=func)`` (keyword-forwarding) and ``deco(**params)(func)`` +#: (factory). +DOUBLED_UP_DECORATORS = (wrap, ch_names, include_exclude, rm_params, add_smart_defaults) + + +def _incr(x, y=1): + """Fixture function to be wrapped by the decorators under test.""" + return x + y + + +@pytest.mark.parametrize("decorator", DOUBLED_UP_DECORATORS, ids=name_of_obj) +def test_double_up_as_factory_accepts_wrapped_by_keyword(decorator): + """``deco(func=func)`` must wrap, not silently make a factory (i2mint/i2#64). + + The failure this guards against is silent: before the fix, passing the object to + wrap by keyword returned a ``functools.partial`` and the caller only found out much + later, at call time, when the "wrapped" object behaved like the decorator instead. + """ + wrapped = decorator(func=_incr) + assert not isinstance(wrapped, partial), ( + f"{name_of_obj(decorator)}(func=...) returned a factory instead of wrapping: " + f"{wrapped!r}" + ) + assert wrapped(2) == _incr(2) == 3 + + +@pytest.mark.parametrize("decorator", DOUBLED_UP_DECORATORS, ids=name_of_obj) +def test_double_up_as_factory_keyword_and_positional_agree(decorator): + """``deco(func)`` and ``deco(func=func)`` must produce equivalent wrappers.""" + from_positional, from_keyword = decorator(_incr), decorator(func=_incr) + assert type(from_positional) is type(from_keyword) + assert from_positional(2) == from_keyword(2) + assert Sig(from_positional) == Sig(from_keyword) + + +@pytest.mark.parametrize("decorator", DOUBLED_UP_DECORATORS, ids=name_of_obj) +def test_double_up_as_factory_still_makes_factories(decorator): + """Guard: the factory direction (no object to wrap) must keep returning a partial.""" + factory = decorator() + assert isinstance(factory, partial) + assert factory(_incr)(2) == 3 + + +def test_double_up_as_factory_with_decorator_params(): + """Guard: giving decorator params (and no wrapped object) still gives a factory.""" + from i2.deco import double_up_as_factory + + @double_up_as_factory + def multiply_result(func=None, *, multiplier=2): + return lambda x: func(x) * multiplier + + assert isinstance(multiply_result(multiplier=3), partial) + assert multiply_result(multiplier=3)(_incr)(2) == 9 + # ... and the two non-factory directions agree + assert multiply_result(_incr, multiplier=3)(2) == 9 + assert multiply_result(func=_incr, multiplier=3)(2) == 9 + + +def test_double_up_as_factory_honors_the_wrapped_params_name(): + """The keyword to use is whatever the decorator named its first param.""" + from i2.deco import double_up_as_factory + + @double_up_as_factory + def decorate(obj=None, *, suffix="!"): + return lambda: obj() + suffix + + hello = lambda: "hello" + assert decorate(obj=hello)() == "hello!" + assert decorate(hello)() == "hello!" + assert isinstance(decorate(suffix="?"), partial) + # ``func`` is NOT special: only the decorator's own first param name is understood + # as "the object to wrap", so ``func=`` stays an (here, unexpected) decorator arg, + # making a factory that complains only when it's used -- as it did before too. + unexpected_kwarg_factory = decorate(func=hello) + assert isinstance(unexpected_kwarg_factory, partial) + with pytest.raises(TypeError): + unexpected_kwarg_factory(hello) + + +def test_double_up_as_factory_rejects_duplicate_wrapped(): + """Giving the wrapped object both positionally and by keyword is an error.""" + from i2.deco import double_up_as_factory + + @double_up_as_factory + def decorate(func=None, *, multiplier=2): + return lambda x: func(x) * multiplier + + with pytest.raises(TypeError): + decorate(_incr, func=_incr)