constants cannot be attributes of another class

[exarkun]

This behavior is surprising and a bit unfortunate:

>>> from twisted.python.constants import Names, NamedConstant
>>> 
>>> class Foo(Names):
...     x = NamedConstant()
... 
>>> class Bar(object):
...     a = Foo.x
... 
>>> Bar.a
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/exarkun/Projects/Twisted/trunk/twisted/python/constants.py", line 40, in __get__
    cls._initializeEnumerants()
AttributeError: type object 'Bar' has no attribute '_initializeEnumerants'
>>> Bar().a
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/exarkun/Projects/Twisted/trunk/twisted/python/constants.py", line 40, in __get__
    cls._initializeEnumerants()
AttributeError: type object 'Bar' has no attribute '_initializeEnumerants'
>>> 

People generally expect to be able to take an object and make it an attribute of a class like this.

Perhaps NamedConstant needs to check the type of the class it's being looked up on? Alternatively, maybe the experiment in not using metaclasses is a failure and twisted.python.constants needs to be switched to using metaclasses.

[exarkun]

(In [37050]) Branching to 'constants-foreign-attribute-5797'

[exarkun]

 Build results

[tom.prince]

1. The comment in test_initializedOnce is no longer true.
2. There don't appear to be tests that other user-defined attributes aren't accessed by lookupByName.
3. Is there any reason for support _ConstantsContainerType instances that don't have '_constantType? In any case, it appears that, despite the docstring, it only supports ones that have _constantType of None`.
   • I guess this is used for the base class _ConstantsContainer.
   • It occurs to me that most of the behavior (except __new__) could probably be implemented as instance methods on _ConstainsContainerType instead of class-methods on _ContainerClass.
4. What is going to happen if somebody tries to assign a constant as an additional attribute on a different constant-container of the same type. I believe that things will silent break. (Please file a ticket about dealing with this case)
5. There don't appear to be any tests about having additional attributes on constant-containers, although there is code to support that case. (Please file a ticket for adding the relevant tests).

Please resubmit for review after addressing the above issues.

[exarkun]

Thanks for the review.

The comment in test_initializedOnce is no longer true.

Fixed in r37313 and r37314.

There don't appear to be tests that other user-defined attributes aren't accessed by lookupByName.

test_notLookupMissingByName covers this. I wonder what additional coverage you're looking for.

Is there any reason for support _ConstantsContainerType instances that don't have '_constantType? In any case, it appears that, despite the docstring, it only supports ones that have _constantType of None`.

Fixed in r37315.

I guess this is used for the base class _ConstantsContainer.

Yes.

It occurs to me that most of the behavior (except new) could probably be implemented as instance methods on _ConstainsContainerType instead of class-methods on _ContainerClass.

Probably. I'd rather not bother changing it, though. It works as-is.

What is going to happen if somebody tries to assign a constant as an additional attribute on a different constant-container of the same type. I believe that things will silent break. (Please file a ticket about dealing with this case)

This is a somewhat bad behavior, I didn't want to introduce this change without fixing it. Fixed it in r37317.

There don't appear to be any tests about having additional attributes on constant-containers, although there is code to support that case. (Please file a ticket for adding the relevant tests).

If I comment out the isinstance(descriptor, cls._constantType) check in __new__, the test module fails even to import. is that the support code you're talking about, or is there something else?

[tom.prince]

The changes looks good. Please merge.

There don't appear to be tests that other user-defined attributes aren't accessed by lookupByName.

test_notLookupMissingByName covers this. I wonder what additional coverage you're looking for.

There don't appear to be any tests about having additional attributes on constant-containers, although there is code to support that case. (Please file a ticket for adding the relevant tests).

If I comment out the isinstance(descriptor, cls._constantType) check in new, the test module fails even to import. is that the support code you're talking about, or is there something else?

None of the _ConstantContainer subclasses in the tests have any explicit attributes other than constants. Something like

!python
class Test(Names):
    bar = "Some text"
    @classmethod
    def baz(cls, arg):
       doStuff()

but doing that is explicitly suggested in the howto, There should be some explicit tests that those work as expected.

[exarkun]

(In [37320]) Give METHOD another random attribute and verify it is not looked up by lookupByName

refs #5797

[tom.prince]

The changes looks good. Please merge.

[exarkun]

(In [37325]) Merge constants-foreign-attribute-5797

Author: exarkun Reviewer: tom.prince Fixes: #5797

Allow constant values defined using twisted.python.constants to be set as attributes of other classes and then be retrieved again via attribute access.