mx-moth
commented
2 months ago Some thoughts: Conventions only change between version updates. Instead of providing a patch for the polygon to _make_polygons() change, it might make more sense to provide a patch for emsarray 0.7.0 to 0.8.0. If this is the approach taken then autodetecting the missing methods feels like a cludge compared to plugins declaring which version of emsarray they were written against. Something like:
@register_convention(interface_version='0.7.0')
class SomeConvention(Convention):
... However this feels aesthetically displeasing.
After #156 plugins such as emsarray-smc will be broken because they no longer implement all of the required abstract methods. Ultimately the plugin should be updated to correctly implement the new interface, but until that happens it would be polite if the plugin could still be used given this is a backwards incompatible change otherwise. This pull request is a work in progress towards implementing Convention hotfixes for changes such as these.
In the case of #156 a functioning Convention subclass can be made from the previous implementation using
Convention.polygonsand somesuper()call shenanigans.Actually getting this hotfix in place is a bit tricky. If an Abstract Base Class is not fully implemented then an error is thrown in object.new at instantiation time. The class itself needs patching before any instances can be instantiated. Specific Convention subclasses can be instantiated in two different ways: automatically by emsarray through autodetecting the appropriate Convention subclass from all the registered conventions, and manually constructing and binding a Convention subclass to a dataset.
Doing metaclass magic isn't possible. The metaclass magic has no way of knowing whether the subclass is final or not. It could be an intermediary subclass such as CFGrid which is further specialised. Patching the CFGrid class would be inappropriate as the CFGrid1D or CFGrid2D classes may provide the implementations.
One approach is to patch the class at the time the class is registered only works for automatically instantiated classes as these go through the registry. This approach has the added benefit of being backwards compatible with the existing Convention plugins which might need patching without any modification. This approach doesn't work for manually instantiated convention classes though, as the class as defined in a module has not been modified. Mutating the classes in place sounds like a bad idea.
Another approach is to add a new decorator that can modify the Convention subclass at the time the class is declared. This would effectively be the same as decorating the class with the
hotfix_convention()method defined in this WIP pull request. A better name would have to be found in that case. The decorator could also register the convention class. This approach has the downside of needing an update to plugins in order to enable the backwards compatible fixes, which kind of negates the point at least for this round of changes, but does offer more power going forward. Also, it is the only option that actually works in all cases.