Decorators not worth the effort

[Ulrich Eckhardt]

Am 14.09.2012 11:28, schrieb Jean-Michel Pichavant:
> Decorators are very popular so I kinda already know that the
> fault is mine. Now to the reason why I have troubles writing
> them, I don't know. Every time I did use decorators, I spent
> way too much time writing it (and debugging it).
>
> I wrote the following one, used to decorate any function that access
> an equipment, it raises an exception when the timeout expires. The
> timeout is adapted to the platform, ASIC of FPGA so people don't need
> to specify everytime one timeout per platform.
>
> In the end it would replace
>
> def boot(self, timeout=15):
>      if FPGA:
>          self.sendCmd("bootMe", timeout=timeout*3)
>      else:
>          self.sendCmd("bootMe", timeout=timeout)
>
> with
>
> @timeout(15)
> def boot(self, timeout=None):
>      self.sendCmd("bootMe", timeout)
>
> I wrote a nice documentation with sphinx to explain this, how to use
> it, how it can improve code. After spending hours on the decorator +
> doc, feedback from my colleagues : What the F... !!

Quite honestly: I think like your colleagues in this case and that in 
this case the decorator doesn't improve the code. Instead, I would 
probably have added a _get_timeout() function that takes care of 
adjusting the argument passed to the function according to the 
underlying hardware target.

To be less abstract, the particular problem I have with your approach is 
that I can't even guess what your code means, let alone what parameters 
it actually takes. If you had written

   @default_timeout(15)
   def boot(self, timeout=None):

instead, I would have been able to guess. OTOH, then again I would have 
wondered why you used a decorator to create a default argument when 
there is builtin support for specifying default arguments for functions.

Maybe you could get away with a decorator like this:

   @adjust_timeout
   def boot(self, timeout=2.5):

The idea is that the decorator modifies the timeout value passed to the 
function (or maybe just modifies the default value?) according to the 
underlying hardware.


> Decorators are very python specific (probably exists in any dynamic
> language though, I don't know), in some environment where people need
> to switch from C to python everyday, decorators add python magic that
> not everyone is familiar with.

The same could be said for classes, iterators, significant whitespace, 
docstrings, lambdas. I think that this was just a bad example but it 
doesn't prove that decorators are worthless. Decorators are useful tools 
if they do something to a function, like doing something before or after 
the actual code, or modifying the context in which the code is called. 
Just setting a default parameter is possible as you have proved, but 
it's IMHO not a good use case.

A bit more specific to your case, adding a timeout decorator would 
actually make much more sense if it transparently invoked the actual 
function in a second thread and the calling thread stops waiting for 
completion and raises an error after that timeout. This has the distinct 
advantage that the code doing the actual communication doesn't have any 
timeout handling code inside.

I'm currently doing something similar here though I only monitor a TCP 
connection that is used for some telnet-style requests. Every function 
making a request over TCP is decorated with @_check_connection. That 
decorator does two things:
1. It checks for an existing fatal connection error.
2. It runs the request and filters resulting errors for fatal connection 
errors.

The decorator looks like this:

def _check_connection(fn):
     @functools.wraps(fn)
     def wrapper(self, *args, **kwargs):
         # check for sticky connection errors
         if self._connection_error:
             raise self._connection_error
         # run actual function
         try:
             return fn(self, *args, **kwargs)
         catch RequestFailed:
             # The other side signalled a failure, but
             # further requests can still succeed.
             raise
         catch ConnectionError, e:
             # The connection is broken beyond repair.
             # Store sticky connection and forward.
             self._connection_error = e
             raise
     return wrapper

I have had other programmers here write such requests and they blindly 
copied the decorator from existing code. This works because the code 
inside that converts/formats/parses the inputs and outputs is completely 
unaware of the connection monitoring. Otherwise, I don't think anyone 
could explain what this decorator does, but they don't have to 
understand it either. It just works.

I wish you a nice weekend!

Uli