0

Is adding an argument to a function through a wrapper a python anti-pattern? I want to add a wrapper that saves the output of many functions to a location, so a wrapper seems to make sense. However, Pycharm fails to autocomplete the arguments to a decorated function (https://intellij-support.jetbrains.com/hc/en-us/community/posts/360002754060-Autocomplete-with-arguments-for-decorated-functions).

and some of the discussion related to changing the function signature with a wrapper seems to indicate that it is a bad practice (https://youtrack.jetbrains.com/issue/PY-33688#focus=Comments-27-3268273.0-0).

Some decorators could change functions' signatures so to correctly process such cases PyCharm have to read decorator's body that could not be done due to performance reasons.

So would doing something like the following be an anti-pattern:

from functools import wraps
from typing import Callable

my_dict = {"very": {"deeply": {"nested": {"filepath": "hidden_filepath"}}}}


def decorator(function: Callable):
    @wraps(function)
    def wrapper(extra_arg: str, function_arg: str) -> str:
        file_path: str = my_dict["very"]["deeply"]["nested"][extra_arg]
        print(f"saving to: {file_path}")
        result: str = function(function_arg)
        print(f"result: {result}")
        return result

    wrapper.__doc__ += "/n:param extra_arg: an extra argument"
    return wrapper


@decorator
def my_function(an_arg: str) -> str:
    """
    my docstring
    :param an_arg:
    :return:
    """
    print(f"my_function arg: {an_arg}")
    return an_arg * 2


my_function("filepath", "cool_str")

I'm also not in love with appending to a docstring in the function, but found that as a solution here: Signature-changing decorator: properly documenting additional argument. Would it make more sense to just change the docstring of the decorated function?

Edit: The only other reasonable solution I can think of is to create a function that takes the other function as an argument, which is what a wrapper is supposed to solve, eg.

def decorator(extra_arg:str, function: Callable, **kwargs)-> str:
    file_path: str = my_dict["very"]["deeply"]["nested"][extra_arg]
    print(f"saving to: {file_path}")
    result: str = function(**kwargs)
    print(f"result: {result}")
    return result

def my_function(an_arg: str) -> str:
    """
    my docstring
    :param an_arg:
    :return:
    """
    print(f"my_function arg: {an_arg}")
    return an_arg * 2

decorator("filepath", my_function, an_arg="cool_str")
Mason Caiby
  • 1,846
  • 6
  • 17
  • 1
    Not sure about the "anti-pattern" part but if I see code like that I would probably refactor it. Too much behind-the-scenes magic going on there. – rdas Jan 06 '21 at 04:36
  • @rdas, see my edit, is that what you would refactor to? It doesn't seem like a superior solution, you don't get any parameter hints from your IDE, refactoring isn't IDE assisted, etc. – Mason Caiby Jan 06 '21 at 04:56
  • Depends on your actual program but I don't prefer seeing each function call being proxied via another. There are probably simpler ways of doing what you're trying to do but that needs more details about the specifics. – rdas Jan 06 '21 at 04:59
  • @rdas, thanks, agree. I'm essentially trying to do the above. Save the results of any random function to a file whose path is stored in a deeply nested dictionary. I'd also like to have some print statements around the function calls (i'm currently tied to print() as my logger) – Mason Caiby Jan 06 '21 at 05:03

1 Answers1

2

Think about maintainability.

Someone else maintaining your code will see that my_function has just one arg. PyCharm and mypy will scream that calls to my_function with more than one arg are wrong. That someone else then go in 'fixing' all the 'bugs'.

Your program breaks.

Hours and hours of troubleshooting before finding out that your decorator changed the signature of the function.

Heck, doesn't need to be someone else... leave your code for a month or two, and when you go back, you'll likely have forgotten that your decorator mangled your function...

So yeah, it's a bad practice, an anti-pattern, a code smell, a <insert your favorite negative-connotation buzzword here>.

pepoluan
  • 6,132
  • 4
  • 46
  • 76
  • Thanks, but it seems like using a wrapper that adds an argument would solve a lot of common problems, like logging results to a file, etc. Is there a better solution that I'm missing? – Mason Caiby Jan 06 '21 at 04:46
  • ??? I honestly can't see why you need an additional arg to log result to a file ... – pepoluan Jan 06 '21 at 04:48
  • you would need the file to log to. – Mason Caiby Jan 06 '21 at 04:52
  • Why? You can just do `logger = logging.getLogger("somename")` inside the function. Let the initialization part of your program do the logging system configuration. A Logger object of the same name is a singleton; no matter where it's getted, they will share the same handlers, etc. – pepoluan Jan 06 '21 at 04:57
  • it was more of an example it's not really core to the discussion. but say you wanted to log to different arbitrary files, and you want to log the execution time of whatever function. so you need to always call the function wrapped in a timer block and need the flexibility of different target files. – Mason Caiby Jan 06 '21 at 05:01
  • It might not be core, but everything can be done in a different way without requiring one to change the signature. In your example there, the wrapper can be the one retrieving the Logger object. Set the Logger config before calling the (wrapped) function, and the wrapper simply use the Logger object without needing to know the Logger's details. – pepoluan Jan 06 '21 at 05:04