Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new hotfix module to automatically handle some Convention updates #157

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mx-moth
Copy link
Contributor

@mx-moth mx-moth commented Sep 10, 2024

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.polygons and some super() 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.

The only current hotfix is to adapt old classes for the renaming of
`Convention.polygon` to `Convention._make_polygons()`.
@mx-moth mx-moth self-assigned this Sep 10, 2024
@mx-moth
Copy link
Contributor Author

mx-moth commented Sep 10, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant