Re: What should a decorator do if an attribute already exists?

[Chris Angelico]

On Tue, May 17, 2016 at 3:05 PM, Steven D'Aprano
 wrote:
> Although the instrumentation is used inside the decorator, it is actually part
> of the public API for the function. So the user can do this:
>
>
> @decorate
> def myfunction():
> ...
>
>
> # later
> myfunction.instrument.query()
>
> and see what data has been collected.
>
> So it is important that the name of the attribute be a public name.

Sounds to me like a collision is largely not your problem, so I'd go
with just tossing in a simple warning as a courtesy. There are plenty
of situations where that courtesy wouldn't be given, and the caller
would have to cope with the clash, so this would already be one up on
that. Not worth going to a heap of hassle to deal with it, as most
functions don't grow new attributes at a great rate. A quick check and
a warning, and that's plenty.

ChrisA
-- 
https://mail.python.org/mailman/listinfo/python-list

Re: What should a decorator do if an attribute already exists?

[Steven D'Aprano]

On Monday 16 May 2016 23:06, Kevin Conway wrote:

>> I have a decorator that adds an attribute to the decorated function
> 
> I might try to argue that this is not actually a decorator or, at least,
> this is not a great decorator pattern for Python. Adding the attribute to
> the function object implies you need to access it at some later point. If
> so then your decorator has changed the public interface of the wrapped
> item. Downstream code will depend on the new interface which is only
> provided by the decorator so it becomes a hard requirement.

This analysis is correct, apart from the value judgement "this is not a great 
decorator pattern".


> IMO, decorators are supposed to be transparent. Removing a decorator should
> not invalidate other code.

Consider the three most obvious decorators in the language: property, 
classmethod and staticmethod. What happens if you remove them?


> With your particular example, I believe you are actually implementing a
> context manager and the more appropriate code would look like:
> 
> with Instrument() as instrument:
> #  other code

It is funny you say that, because my project offers a context manager interface 
as well.

The context manager interface is based on this recipe:

http://code.activestate.com/recipes/577896

but it adds a decorator interface so you can decorate a function, run the 
function as often as you want, then look at the timing statistics.


-- 
Steve

-- 
https://mail.python.org/mailman/listinfo/python-list

Re: What should a decorator do if an attribute already exists?

[Steven D'Aprano]

On Monday 16 May 2016 22:20, jmp wrote:

> On 05/10/2016 05:45 PM, Steven D'Aprano wrote:
>> I have a decorator that adds an attribute to the decorated function:
> [snip]
>> I think 5 is clearly wrong, 4 is too difficult, and 3 seems pointless. So I
>> think either 1 or 2 is the right thing to do.
>>
>> Thoughts?
> 
> It depends if the attribute "instrument" is part of the public interface.

Yes it is.

> If not, just find a different name unlikely to clash with an existing
> one *and* raise an exception if it does ever happen anyway.
> 
> If the attribute is part of the public interface but you re using the
> code only internally then 1/ raise an exception, otherwise I don't know :o)
> 
> It seems to me that over-writing silently (or not) may lead to bugs
> difficult to spot.

At this point, I think I will raise a warning. The user can use the warnings 
module to turn that warning into an error, if they want to, otherwise it's not 
necessarily an error.



-- 
Steve

-- 
https://mail.python.org/mailman/listinfo/python-list

Re: What should a decorator do if an attribute already exists?

[Steven D'Aprano]

On Monday 16 May 2016 18:14, Francesco Loffredo wrote:

> On 10/05/2016 17:45, Steven D'Aprano wrote:
>> I have a decorator that adds an attribute to the decorated function:
[...]
>> My question is, what should I do if the decorated function already has an
>> instrument attribute?

>  From your example, it seems that you use your instrument only inside
> your decorator. So I think "instrument" could be a "private variable".

My example was misleading, sorry about that.

Although the instrumentation is used inside the decorator, it is actually part 
of the public API for the function. So the user can do this:


@decorate
def myfunction():
...


# later
myfunction.instrument.query()

and see what data has been collected.

So it is important that the name of the attribute be a public name.


> What if you called your instrument "__instrument", taking advantage of
> name mangling?

Double underscore name-mangling only works inside classes.

[...]
> And what happens if you want to add another instrument, decorating the
> target twice?

If the names are the same, the second decorator will over-write the first 
decorator's instrumentation with its own.




-- 
Steve

-- 
https://mail.python.org/mailman/listinfo/python-list

Re: What should a decorator do if an attribute already exists?

[Kevin Conway]

> I have a decorator that adds an attribute to the decorated function

I might try to argue that this is not actually a decorator or, at least,
this is not a great decorator pattern for Python. Adding the attribute to
the function object implies you need to access it at some later point. If
so then your decorator has changed the public interface of the wrapped
item. Downstream code will depend on the new interface which is only
provided by the decorator so it becomes a hard requirement.

IMO, decorators are supposed to be transparent. Removing a decorator should
not invalidate other code.

With your particular example, I believe you are actually implementing a
context manager and the more appropriate code would look like:

with Instrument() as instrument:
#  other code

On Mon, May 16, 2016, 07:23 jmp  wrote:

> On 05/10/2016 05:45 PM, Steven D'Aprano wrote:
> > I have a decorator that adds an attribute to the decorated function:
> [snip]
> > I think 5 is clearly wrong, 4 is too difficult, and 3 seems pointless.
> So I
> > think either 1 or 2 is the right thing to do.
> >
> > Thoughts?
>
> It depends if the attribute "instrument" is part of the public interface.
>
> If not, just find a different name unlikely to clash with an existing
> one *and* raise an exception if it does ever happen anyway.
>
> If the attribute is part of the public interface but you re using the
> code only internally then 1/ raise an exception, otherwise I don't know :o)
>
> It seems to me that over-writing silently (or not) may lead to bugs
> difficult to spot.
>
> jmp
>
> --
> https://mail.python.org/mailman/listinfo/python-list
>
-- 
https://mail.python.org/mailman/listinfo/python-list

Re: What should a decorator do if an attribute already exists?

[jmp]

On 05/10/2016 05:45 PM, Steven D'Aprano wrote:

I have a decorator that adds an attribute to the decorated function:

[snip]

I think 5 is clearly wrong, 4 is too difficult, and 3 seems pointless. So I
think either 1 or 2 is the right thing to do.

Thoughts?


It depends if the attribute "instrument" is part of the public interface.

If not, just find a different name unlikely to clash with an existing 
one *and* raise an exception if it does ever happen anyway.


If the attribute is part of the public interface but you re using the 
code only internally then 1/ raise an exception, otherwise I don't know :o)


It seems to me that over-writing silently (or not) may lead to bugs 
difficult to spot.


jmp

--
https://mail.python.org/mailman/listinfo/python-list

Re: What should a decorator do if an attribute already exists?

[Francesco Loffredo]

On 10/05/2016 17:45, Steven D'Aprano wrote:

I have a decorator that adds an attribute to the decorated function:


def decorate(func):
 instrument = make_instrument()

 @functools.wraps(func)
 def inner(*args):
 instrument.start()
 result = func(*args)
 instrument.finish()
 return result

inner.instrument = instrument
return inner


The actual nature of the instrumentation isn't important: depending on the
decorator, it might count the number of function calls made, how long it
takes, count cache hits, or something else.

My question is, what should I do if the decorated function already has an
instrument attribute?

1. raise an exception?

2. raise a warning, and over-write the attribute?
3. raise a warning, and skip adding the attribute?
4. raise a warning, and rename the existing instrument to
something else before writing my own instrument?

5. silently over-write the attribute?


I think 5 is clearly wrong, 4 is too difficult, and 3 seems pointless. So I
think either 1 or 2 is the right thing to do.

Thoughts?

CAVEAT: I speak out of utter ignorance, please don't slap me if i'm 
saying something blatantly stupid...


From your example, it seems that you use your instrument only inside 
your decorator. So I think "instrument" could be a "private variable".


What if you called your instrument "__instrument", taking advantage of 
name mangling?
This would, IMHO, solve entirely the name clash problem, and you could 
even access your instrument from outside, using its "mangled" name.
This, of course, leads to another question: what happens to name 
mangling in a decorator? What will be actually called the variable 
"__instrument"?


And what happens if you want to add another instrument, decorating the 
target twice?


Francesco
--
https://mail.python.org/mailman/listinfo/python-list

Re: What should a decorator do if an attribute already exists?

[Michael Selik]

On Tue, May 10, 2016 at 11:48 AM Peter Otten <__pete...@web.de> wrote:

> Steven D'Aprano wrote:
> > I have a decorator that adds an attribute to the decorated function:
> >inner.instrument = instrument
> >return inner
>


> the original instrument is still accessible as f.__wrapped__.instrument


I'd say Peter's example is Option 6:
Wrap the instrumented function with a second instrumented wrapper.

Since you have various kinds of instruments, silently adding another layer
makes sense to me. Is it OK if the order matters? Will some of the
instruments get confused because they're working on a wrapper instead of
the original function?
-- 
https://mail.python.org/mailman/listinfo/python-list

Re: What should a decorator do if an attribute already exists?

[Ethan Furman]

On 05/10/2016 08:45 AM, Steven D'Aprano wrote:


I have a decorator that adds an attribute to the decorated function:



My question is, what should I do if the decorated function already has an
instrument attribute?


If the decorator is adding an attribute for the decorated thing to use, 
and that thing may already have the correct attribute, do nothing; 
otherwise, raise an exception.


--
~Ethan~

--
https://mail.python.org/mailman/listinfo/python-list

Re: What should a decorator do if an attribute already exists?

[MRAB]

On 2016-05-10 17:06, Stephen Hansen wrote:

On Tue, May 10, 2016, at 08:45 AM, Steven D'Aprano wrote:

I have a decorator that adds an attribute to the decorated function:
[...]
My question is, what should I do if the decorated function already has an
instrument attribute?

1. raise an exception?


This. Your decorator should, IMHO, treat the attribute as private data,
and if something else is using the same thing, something has clearly
gone wrong and raising the error early and clearly is right.

If it's not clear what you should do, you could look at the Zen. It 
says: "In the face of ambiguity, refuse the temptation to guess." To me, 
that suggests raising an exception.


--
https://mail.python.org/mailman/listinfo/python-list

Re: What should a decorator do if an attribute already exists?

[Peter Otten]

Steven D'Aprano wrote:

> I have a decorator that adds an attribute to the decorated function:
> 
> 
> def decorate(func):
> instrument = make_instrument()
> 
> @functools.wraps(func)
> def inner(*args):
> instrument.start()
> result = func(*args)
> instrument.finish()
> return result
> 
>inner.instrument = instrument
>return inner
> 
> 
> The actual nature of the instrumentation isn't important: depending on the
> decorator, it might count the number of function calls made, how long it
> takes, count cache hits, or something else.
> 
> My question is, what should I do if the decorated function already has an
> instrument attribute?
> 
> 1. raise an exception?
> 
> 2. raise a warning, and over-write the attribute?
> 3. raise a warning, and skip adding the attribute?
> 4. raise a warning, and rename the existing instrument to
>something else before writing my own instrument?
> 
> 5. silently over-write the attribute?
> 
> 
> I think 5 is clearly wrong, 4 is too difficult, and 3 seems pointless. So
> I think either 1 or 2 is the right thing to do.
> 
> Thoughts?

Silently overwrite it. For

@decorate
@decorate
def f():
pass

the original instrument is still accessible as f.__wrapped__.instrument, so 
5 is the easy way to get 4.

-- 
https://mail.python.org/mailman/listinfo/python-list

Re: What should a decorator do if an attribute already exists?

[Stephen Hansen]

On Tue, May 10, 2016, at 08:45 AM, Steven D'Aprano wrote:
> I have a decorator that adds an attribute to the decorated function:
> [...]
> My question is, what should I do if the decorated function already has an
> instrument attribute?
> 
> 1. raise an exception?

This. Your decorator should, IMHO, treat the attribute as private data,
and if something else is using the same thing, something has clearly
gone wrong and raising the error early and clearly is right.

-- 
Stephen Hansen
  m e @ i x o k a i . i o
-- 
https://mail.python.org/mailman/listinfo/python-list

What should a decorator do if an attribute already exists?

[Steven D'Aprano]

I have a decorator that adds an attribute to the decorated function:


def decorate(func):
instrument = make_instrument()

@functools.wraps(func)
def inner(*args):
instrument.start()
result = func(*args)
instrument.finish()
return result

   inner.instrument = instrument
   return inner


The actual nature of the instrumentation isn't important: depending on the
decorator, it might count the number of function calls made, how long it
takes, count cache hits, or something else.

My question is, what should I do if the decorated function already has an
instrument attribute?

1. raise an exception?

2. raise a warning, and over-write the attribute?
3. raise a warning, and skip adding the attribute?
4. raise a warning, and rename the existing instrument to 
   something else before writing my own instrument?

5. silently over-write the attribute?


I think 5 is clearly wrong, 4 is too difficult, and 3 seems pointless. So I
think either 1 or 2 is the right thing to do.

Thoughts?



-- 
Steven

-- 
https://mail.python.org/mailman/listinfo/python-list