Good interface for reverting effect of function

Question

In my job, we often write functions that do some calculation and write the result to a database. Sometimes mistakes are made and wrong stuff ends up being saved, therefore we also want to supply an interface to clean up after the function. This is an attempt of solving that problem.

from functools import wraps

def revert(func):
    if not hasattr(func,'_revert'):
        raise NotImplementedError(func.__name__ + " does not have a revert registered")

    @wraps(func._revert)
    def wrapper(*args,**kwargs):
        return func._revert(*args,**kwargs)
    return wrapper

def register_revert(revert):
    def revert_added(func):
        func._revert = revert

        @wraps(func)
        def wrapper(*args,**kwargs):
            return func(*args,**kwargs)
        return wrapper
    return revert_added

def _is_it_a_str(maybe_str):
    if not isinstance(maybe_str,str):
         raise ValueError("I need a string")

def _revert_stuff(a_str):
    _is_it_a_str(a_str)
    print('revert stuff with ' + a_str)

@register_revert(_revert_stuff)
def do_stuff(a_str):
    _is_it_a_str(a_str)
    print('doing stuff with ' + a_str)


do_stuff('something')
# Oops:
revert(do_stuff)('something')

The idea is to write your function doing your job - do_something in this case. And then when you are ready to take it to the next level, you write its revert, register it and you are done.

I like that

• the solution does not require changing anything in the main functionality.
• you can use the same identical interface for many functions
• the syntax reads relatively intuitively

I think it is inconvenient that

• I need to maintain two identical interfaces - the one on do_stuff and _revert_stuff needs to be identical (or at least _revert_stuff needs to work getting the one from do_stuff).
• sometimes I want to type-check the arguments which I then also have to repeat (exemplified here with _is_it_a_str). I thought about moving that to another decorator, but I'm afraid it gets too confusing.

Edit: We use the newest stable version of Python

p.s. My first question tried my best. Critique welcome!

Answer 1

def revert(func):
    @wraps(func._revert)
    def wrapper(*args,**kwargs):
        return func._revert(*args,**kwargs)
    return wrapper

You are returning a wrapper, which takes arguments, calls a function, and returns results...

I'm pretty sure that is just a long way of writing:

def revert(func):
    return func._revert

This begs the question: why store the revert function as func._revert, when you could store it as func.revert? Then instead of:

revert(do_stuff)('something')

you could write:

do_stuff.revert('something')

which is one character shorter. Even better: autocomplete can tell you whether there is a .revert member for do_stuff, which is not possible using revert(...).

Answer 2

I don't have any complaints about your code itself, so I'll bring up an alternate approach. You mentioned being annoyed at having to duplicate things like typechecking, and I also notice that it's possible to call revert(do_stuff)("b") without having ever called do_stuff("b") in the first place. Maybe that's desirable, but I feel like it probably isn't.

One way to tackle both those things would be to bind the function's arguments to some undo object, and only make the revert function accessible that way. This way, an undo is guaranteed to undo an action that has been done, with the same arguments (meaning they shouldn't need to be typechecked a second time).

Now, this has some disadvantages but it might be worth considering an approach like

class UndoableAction:
    def __init__(self, revert_func, args, kwargs):
        self._revert_func = revert_stuff
        self._args = args
        self._kwargs = kwargs
        self._has_been_undone = False

    def undo(self):
        if self._has_been_undone:
            raise RuntimeError("Could not undo: Action has already been undone")
        else:
            result = self._revert_func(*(self._args), **(self._kwargs))
            self._has_been_undone = True
            return result


def register_revert(revert_func):
    def bind(original_func):
        def bound_func(*args, **kwargs):
            result = original_func(*args, **kwargs)

            return (result, UndoableAction(revert_func, args, kwargs))

        return bound_func
    
    return bind


def _revert_stuff(a_str):
    # Since this should only be called as some_action.undo()
    # we can assume do_stuff() has already typechecked the argument
    # So we don't do it ourselves
    print('revert stuff with ' + a_str)


@register_revert(revert_stuff)
def do_stuff(a_str):
    if not isinstance(a_str, str):
        raise ValueError("I need a string")

    print('do stuff with ' + a_str)


_, stuff = do_stuff('something')
# Oh no!
stuff.undo()

The syntax remains fairly intuitive, and the same interface still works for many functions. Duplication is down, and while the two functions still need to be kept in sync to a degree, the undo function doesn't end up exposing much of an interface at all. And there is also no need to worry about trying to undo an operation that hasn't actually been done. But of course, it does look different to callers of the main functionality. It also might make it harder to undo an action if the action was initially done in an entirely different part of the program. If you don't have the undo object, you can't easily do something like revert(create_user)(database.get_a_username()) for example -- but that might not be a major issue, assuming the same code is generally responsible for doing a thing and undoing it (which it sounds like).

It's a tradeoff. Might be worth thinking about, depending on your use case.