[MRG] allow child classes to override parents' tags #14069
Conversation
| more_tags = base_class._more_tags(self) | ||
|
|
||
| if '_more_tags' in self.__class__.__dict__: | ||
| # we cannot use getattr since it would resolve to the parent's |
There was a problem hiding this comment.
we could use getattr(type(self), '_more_tags'), right?
There was a problem hiding this comment.
no I think it would also resolve to the parents'
|
Do we really want to overwrite the python inheritance mechanism just so we don't have to fix the way we inherit from mixins? |
| return | ||
|
|
||
| for direct_parent_class in c.__bases__: | ||
| _resolve_tags(direct_parent_class) |
There was a problem hiding this comment.
yeah I suppose there are other ways to wrap it up
No idea yet. I'm just opening this so we have a concrete implementation of what it could look like. |
|
|
||
| # filter-out tags that will be overridden in child class | ||
| more_tags = {key: val for (key, val) in more_tags.items() | ||
| if key not in overridden_tags.keys()} |
There was a problem hiding this comment.
So if you don't do this does the consistency check fails?
Does that generalize to multiple inheritances: A (has some tags) > B (overwrite tags) > (overwrite tags) ?
There was a problem hiding this comment.
We need this because we allow tags that are overridden on the child class to be inconsistent in the parents classes
| overridden_tags = {} | ||
|
|
||
| def _resolve_tags(c): | ||
| nonlocal collected_tags |
There was a problem hiding this comment.
can't you just pass collected_tags as a reference to _resolve_tags?
It's a pretty impressive solution @NicolasHug , but I'm also not so convinced about reimplementing something that behaves like python inheritance but not quite. I think, just using python MRO and fixing the mixin ordering would be a simpler and easier to maintain approach in the long run. |
|
Agreed, closing :) ! |
Fixes #14044
This PR relaxes the current constraint on the tags and implements the solution proposed in #14044 (comment)
That is:
waiting to see if CI goes green