Opened 2 years ago

Closed 17 months ago

#5797 defect closed fixed (fixed)

constants cannot be attributes of another class

Reported by: exarkun Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords:
Cc: hs@… Branch: branches/constants-foreign-attribute-5797
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

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.

Change History (10)

comment:1 Changed 19 months ago by hynek

  • Cc hs@… added

comment:2 Changed 18 months ago by exarkun

  • Author set to exarkun
  • Branch set to branches/constants-foreign-attribute-5797

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

comment:3 Changed 18 months ago by exarkun

  • Keywords review added

comment:4 Changed 18 months ago by tom.prince

  • Keywords review removed
  • Owner set to exarkun
  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.

comment:5 Changed 17 months ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

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?

comment:6 Changed 17 months ago by tom.prince

  • Keywords review removed
  • Owner set to exarkun

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.

comment:7 Changed 17 months ago by exarkun

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

refs #5797

comment:8 Changed 17 months ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

comment:9 Changed 17 months ago by tom.prince

  • Keywords review removed
  • Owner set to exarkun

The changes looks good. Please merge.

comment:10 Changed 17 months ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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.

Note: See TracTickets for help on using tickets.