search

zopefoundation / zope.annotation

Mechanism for storing additional information about objects without modifying their classes
Other
1 stars 8 forks source link

Make AttributeAnnotations have a __parent__ #13

Closed jamadden closed 7 years ago

jamadden commented 7 years ago

Fixes #11 in a BWC way using a property. This makes it more like zope.principalannotation (though I didn't change the interface here).

Also some doc fixes I noticed when I was fixing up the doctest snippets (I originally had this implementing ILocation with a __name__ of __annotations__ but I realized that didn't really make any sense so I backed it out.)

icemac commented 7 years ago

@jamadden I added you as owner of this package on PyPI.

jamadden commented 7 years ago

I considered adding __parent__ to an interface, but in the end it didn't seem worth it to canonicalize it like that. There were two options, either (1) add it to IAnnotations or (2) create a subclass of IAnnotations, say IAttributeAnnotations that adds __parent__ and which zope.annotation.attribute.AttributeAnnotation now implements.

Adding it to IAnnotations would promote what I see as an implementation detail into the general interface. There might be other IAnnotations implementations out there that do not have a __parent__ and they would now be out of compliance with the interface (for example, I could imagine an implementation for external data, like that stored in LDAP, that you get with something like ldap_annotation = ldap_annotations.get('cn=foo,ou=bar,o=baz') which wouldn't meaningfully have a __parent__).

Using a new subclass wouldn't have that problem but maybe it introduces its own compatibility issues (I don't know what they would be, but it seems like there might be some)? For the kind of code I was concerned with, that doesn't check interfaces or use ILocation or anything like that, it wouldn't really add any benefit, either. This is just helping this object comply with an ad-hoc protocol of having a __parent__ chain when possible.

I don't feel super-strongly about that, though, so if anyone does feel strongly like this should be added to an interface, I can do that.