Fix double_up_as_factory when the wrapped object is passed by keyword

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),
+        )
     )
 
 

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)