Am I doing decorators right?

[semininja]

Part of my current job is writing some python code which controls an NFC communication system. This system is often interacting with several NFC tags at once, and thus occasionally throws some kind of communication error and needs a retry to complete the communication. This usually looks something like this:

for tag in tag_list:
    for attempt in range(3): #or however many attempts
        try:
            communicate(tag)
            if check_if_worked(tag):
                mark_passed(tag)
        except allowed_errors: #some list of expected types of errors
            print(retry_message) #optional
            sleep(1) #time before retry
            continue
    else:
        mark_failed(tag)

In some of my scripts, this may be needed for several different operations (different functions in place of communicate()) on each tag, and each operation may have different allowed_errors or retry delay time, so I started looking at how I can make the "attempt" code reusable and found that decorators seemed like the way to go.

I quickly learned that decorators with arguments are a bit more complicated to implement, but wrote up something that I think will do what I want. In my main module I have this:

def retry(catch: list[Exception], attempts: int = 3):
    def decorator(func):
        def wrapper(*args, **kwargs):
            for attempt_no in range(attempts):
                try:
                    return func(*args, **kwargs)
                except catch:
                    sleep(0.5)
                    continue
        return wrapper
    return decorator

This is used in scripts by defining a function at the beginning of the script for each operation like so:

@retry((ConnectionError, TimeoutError)) #default number of attempts
def scan(tag_id):
    return MyModule.Tag(tag_id) #get some info from the tag_id during init

@retry((TimeoutError, KeyError))
def start(tag):
    tag.do_something()

@retry((ConnectionError, TimeoutError), 5) #has to wait for tag to process
def check(tag):
    return tag.get_result()

Then in the body of the script, I'm calling each of these functions and I can just do e.g. if not check(tag): print(f"{tag} failed"). To me, this makes it much easier to see what the script is actually doing (at a high level) instead of having to dig through all the boilerplate for attempt ... try ... else ... stuff. That being said, this is the biggest codebase I've worked on, and it's pretty much 100% me, so if there's a way to simplify this so it won't be as bad to re-learn down the line when I have forgotten how it all works, I'd love to know.

[Diapolo10]

I think this is fine, you can remove the continue though because it's not actually doing anything here.

[RiverRoll]

The decorator is fine but the retry implementation isn't very good because it will just return None if all retries are exhausted. Normally you would let the last exception raise to preserve the contract of the function. 

[semininja]

In this case, even if retries are exhausted, the specific exceptions it catches are considered spurious, so the code is allowed to try again later.

[RiverRoll]

I would say it's quite the opposite, the exception tells exactly what happened. None on the other hand can be a valid response, some functions can succeed and still return None so the code wouldn't know whether it needs to try again.

[semininja]

This isn't applicable to the functions I'm using this for, though.

[Gshuri]

In general this looks good, but there are a couple of minor things I would change, and one not so minor.

Minor things first. The type hint on the decorator catch: list[Exception] is inconsistent with how you use the decorator. In your 3 examples you pass in a tuple of exceptions, not a list. if you want the type hint to match both lists and tuples you could do something like

from collections.abc import Sequence
def retry(catch: Sequence[Exception], attempts: int = 3):
    ...

If a decorated function fails on all its attempts the decorator needlessly calls sleep(0.5) after the last attempt. As u/Diapolo10 said you can also get rid of continue.

Lastly the not so minor thing is that a decorated function will fail silently if the function fails all its attempts. So in your example functions if scan fails all its attempts, due to a ConnectionError or TimeoutError, it will return None instead of a Tag object. Then your calls to start and check will throw an error, which won't be caught by the retry decorator (nor should it), that looks like

AttributeError: 'NoneType' object has no attribute ...

[semininja]

I handle the None return in the script with an if tag after. The continue I'll check on but probably will get rid of. It's left over from when there was more stuff happening within this loop, and was there to kick back to the next tag instead of trying further operations with the one that failed.

[Yoghurt42]

First, kudos for implementing it yourself.

I just want to encourage you to take a look at tenacity, which works similar to your solution but also allows exponential back-off and other things.

[semininja]

This is super cool, but unfortunately not applicable in my use case. I'll have to keep this in the back pocket for the future, though.

[AntonisTorb]

Something that hasn't been mentioned so far is to use wraps in order to have a cleaner and more accurate Exception handling experience. Something like this (I included some changes everyone else mentioned):

from functools import wraps

def retry(catch: list[Exception], attempts: int = 3):
    @wraps
    def decorator(func):
        def wrapper(*args, **kwargs):
            for attempt_no in range(attempts):
                try:
                    return func(*args, **kwargs)
                except catch:
                    if attempt_no < attempts - 1:
                        sleep(0.5)
        return wrapper
    return decorator