provides taggedvalue inheritance

[mamico]

provides query/get/list taggedvalue with subclass inheritance
fix: plone/plone.dexterity#57 and similar issues

[mgedmin]

LGTMish, but I don't see

• a changelog update
• documentation update

Also, is this a backwards-incompatible change (requiring a major version bump), a new feature (requiring a minor version bump), or a bugfix?

[mamico]

LGTMish,

good, thanks.

but I don't see

• a changelog update

done.

• documentation update

Also, is this a backwards-incompatible change (requiring a major version bump), a new feature (requiring a minor version bump), or a bugfix?

I ask for your support on that. IMHO it's a bugfix, no changes. The only backwards-incompatible could be if someone expects the current wrong behavior, where tag on a class don't inherits in a subclass...

[mgedmin]

LGTM

(I'm not sure my review is worth much, given that I've no idea this API existed in zope.interface, despite using it for many years ;)

[icemac]

LGTM.

I think of this change not as a simple bugfix more as a missing feature as you only had to write new code instead of changing existing one (besides in the tests.)

[icemac]

Coveralls seems to be broken in itself: it lists all files several times.

[mgedmin]

I think of this change not as a simple bugfix more as a missing feature as you only had to write new code instead of changing existing one (besides in the tests.)

There's a difference between overriding an inherited method (to fix behaviour) and adding a new method (to add behaviour), but merely looking at the diff is not enough to distinguish these two cases (which was why I asked).

[icemac]

@mgedmin Right, I did not look deep enough into the code.

I know and use the tagging features sporadically. The issue which is fixed here has never been a problem for me.
I am not sure if this PR breaks someone's code so let's make the screaming test.

[mamico]

@icemac @mgedmin thanks a lot for the reviews.
This i not an urgent bug, in my opinion, but a bit annoying on cases like plone/plone.dexterity#57 filled by @volkerjaenisch and @jensens.
I ran into the same problem and wasted time to know the cause.
BTW plone dexterity uses extensively schema tagging for field permission, sorting, ...

[jensens]

This missing inheritance of tagged values always felt like a bug to me, even if one may argue its a missing feature. It is just not what one would expect. Also, it makes life harder than it should.

👍 for merging this.

[icemac]

@mamico Feel free to merge the PR yourself to prove you signed the Contributor agreement.

[icemac]

@mamico Do you need a release of zope.interface to use this feature somewhere?

[mamico]

@mamico Do you need a release of zope.interface to use this feature somewhere?

@icemac as I said above I've workaround. No particular urgency for me. Thanks.

[jamadden]

FWIW, this turned out to be a breaking change for us. We apparently had code (in nti.externalization) that implicitly relied on tagged values not being inherited. There's code that examines classes in a module, and if they implement an interface that provides a particular tagged value, a utility is registered with a name from the value of that tag. With 4.7.0, we started registering duplicate utilities because we started finding classes that only implemented a derived interface. Simplified example:

class IPublic(Interface):
    taggedValue('tagged-value', 'public')

class IPrivate(IPublic):
    pass

@implementer(IPublic)
class Public(object): pass

@implementer(IPrivate)
class PrivateImplementation(object): pass

With 4.6, we'd register Public with a name of public. With 4.7, we're trying to register Public with a name of public and PrivateImplementation also with a name of public. This leads to zope.configuration.config.ConfigurationConflictError: Conflicting configuration actions.

I think I can workaround this by calling Element.queryTaggedValue(iface, ...) instead of iface.queryTaggedValue(...)...

(EDIT: Clarified some typos in the sample code.)