mkorpela / overrides

A decorator to automatically detect mismatch when overriding a method
Apache License 2.0
261 stars 32 forks source link

Support for overriding inner classes #101

Closed jobh closed 2 years ago

jobh commented 2 years ago

In some patterns, overriding inner classes is desireable. The inner class does not have __globals__, so explicitly specifying the superclass is necessary (I think). Example:

class Base:
    class Params:
        pass

class Derived(Base, EnforceOverrides):
    @overrides(super_class=Base)
    class Params:
        pass
mkorpela commented 2 years ago

Thanks for the contribution @jobh. Unfortunately I do think that there must be away to solve this without the explicit super class definition. Could you try to find one? I think the problem you describe (overriding inner class) is a valid one and should be possible.

jobh commented 2 years ago

Hi @mkorpela, unfortunately it seems to be impossible to do un-intrusively, since the inner class is not bound to the outer class.

And the outer class is still being constructed, so it does not exist in the module yet and can't be looked up to find its superclass.

It would probably be possible for a metaclass to access/inject the mapping after construction of the outer class though. That would require the checking to be similarly postponed, so it's probably not worthwhile to pursue (imo). If you think it might work out (both interface-wise to require metaclass, and architecturally wrt postponing the check) I can try.

[edit: Found a possible trick using object.__set_name__, https://stackoverflow.com/a/54316392/12689370. Requires py 3.6+ but that seems ok.]

jobh commented 2 years ago

No: __set_name__ looked promising. It would also simplify super class lookup by having the owner class (and its mro) directly available. However: All exceptions are re-raised as RuntimeError, which doesn't make anyone happy.

So: Options seem to be

  1. Explicit superclass
  2. Required metaclass
  3. RuntimeError with TypeError as cause

Implementation is here, https://github.com/mkorpela/overrides/compare/main...jobh:overrides:experiment?expand=1. I don't think it's worth pursuing further.

mkorpela commented 2 years ago

Thanks @jobh for taking the time and effort in investigating.

jobh commented 2 years ago

A pleasure! For your edification, here's what it might look like with metaclass. It's just proof-of-concept, it should probably be refactored as a another metaclass separate from the enforcing one, etc.

https://github.com/mkorpela/overrides/compare/main...jobh:overrides:metaclass?expand=1

Note, the metaclass does just the same as the default (__set_name__), just without wrapping the exception

mkorpela commented 2 years ago

This seems to work

for super_class in _get_base_classes(sys._getframe(3), vars(sys.modules[method.__module__])):
mkorpela commented 2 years ago

I really value the problem description and the tests and your effort and would like to release changes to support overriding inner classes. @jobh would you be willing to do an implementation where the _get_base_classes second argument namespace would be in this class case be vars(sys.modules[method.__module__]). To minimise risks I would also want the namespace be method.__globals__ when that exists.

jobh commented 2 years ago

Hey, that was easy! ;-)

I'm surprised though, since I didn't find the parent class in the module dict. Is there a difference between sys.modules and the "plain" module scope, or did I just miss it? (the latter is always possible, don't feel bad about saying so)

Anyway, pushed a change reverting my original and just did the minimal namespace change. Let me know if you think it needs cleaning up.

jobh commented 2 years ago

Oh and btw; on my other branches I did a thing with comparing __init__ signatures, I don't know if that's worth keeping or not. It doesn't work with slot wrappers.

mkorpela commented 2 years ago

Thank you for the contribution. I try to get this released this week.