the bug is that unregister_attribute() is not working, which the test
suite is using to remove and re-register new instrumentation:
class Foo(object):
pass
attributes.register_attribute(Foo, "collection",
uselist=True, typecallable=set, useobject=True)
assert
attributes.get_class_state(Foo).is_instrumented("collection")
attributes.unregister_attribute(Foo, "collection")
assert not
attributes.get_class_state(Foo).is_instrumented("collection")
seems like unregister_attribute() is still doing the old thing of just
"del class.attr", so we'd just need to stick a hook similar to
"instrument_attribute" for this on ClassState which will take over the
job.
looks good !
On Mar 3, 2008, at 1:39 PM, Phillip J. Eby wrote:
> At 04:04 PM 2/27/2008 -0500, Michael Bayer wrote:
>> do you have any interest in committing changes to the branch
>> yourself ? as long as the unit tests keep running whatever you'd
>> want
>> is most likely fine with me....otherwise I will at least experiment
>> with doing away with __mro__ searching and possibly doing away with
>> pre_instrument_attribute.
>
> Okay, I've run into an interesting hitch on the branch -- I think
> it's a test coverage issue. The definition of
> 'ClassState.is_instrumented' is different from that of
> _ClassStateAdapter, in a way that appears to *matter*. That is, if
> you change ClassState.is_instrumented to work in roughly the same way
> as _ClassStateAdapter.is_instrumented, the standard tests
> fail. Specifically, with this rather unhelpful error... :)
>
> Traceback (most recent call last):
> File "test/orm/attributes.py", line 397, in test_collectionclasses
> assert False
> AssertionError
>
> This would appear to indicate that there is some kind of hidden
> dependency between collections support, and the presence of SA
> descriptors on a class.
>
> Further investigation reveals that the problem comes from
> register_attribute() checking to see if an attribute is already
> instrumented, and if so, returns immediately. It would appear that
> this really wants to know if there is actually an instrumented
> descriptor, rather than whether one has merely been registered with
> the class state, per these comments::
>
> # this currently only occurs if two primary mappers are made for the
> same class.
> # TODO: possibly have InstrumentedAttribute check "entity_name" when
> searching for impl.
> # raise an error if two attrs attached simultaneously otherwise
>
> I'm pretty much stumped at this point. Essentially, even though the
> tests don't cover this point for custom instrumentation, it means
> that this problem *will* occur with custom instrumentation, even
> without my changes.
>
> Now, the *good* news is, the other 1190 tests all pass with my
> changes. :)
>
> I've attached my current diff against the user_defined_state
> branch. Note that this bit is the part that makes the one test break:
>
> def is_instrumented(self, key, search=False):
> if search:
> - return hasattr(self.class_, key) and
> isinstance(getattr(self.class_, key), InstrumentedAttribute)
> + return key in self
> else:
> - return key in self.class_.__dict__ and
> isinstance(self.class_.__dict__[key], InstrumentedAttribute)
> + return key in self.local_attrs
>
> In other words, it's only the change to ClassState.is_instrumented
> that breaks the test, showing that there's a meaningful difference
> here between what gets registered in the state, and what's actually
> on the class. Which is going to be a problem for anybody rerouting
> the descriptors (the way the Trellis library will be).
>
> >
> <classstate_dict.patch>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"sqlalchemy" group.
To post to this group, send email to sqlalchemy@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/sqlalchemy?hl=en
-~----------~----~----~----~------~----~------~--~---
