-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add abstractmethods to backend classes #7460
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
base: main
Are you sure you want to change the base?
Conversation
|
This seems like a good idea to me, but I don't know much about this part of the codebase. We should at minimum state in the docstring of these method whether they are required or optional. From the errors thrown in the tests it seems |
jhamman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. Two small comments.
| txt += f"\n Learn more at {self.url}" | ||
| return txt | ||
|
|
||
| @abstractmethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per #7437 (comment), we may not want to require all of these methods. We may end up in a situation where BackendEntrypoints must define one or more of open_dataarray, open_dataset, and open_datatree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems pretty doable, e.g.
class MyBaseClass:
def __init__(self):
super().__init__()
# Check that at least one of the methods has been overridden
if all(getattr(self.__class__, m) is getattr(MyBaseClass, m) for m in ['method1', 'method2', 'method3']):
raise TypeError("You must implement at least one of 'method1', 'method2', 'method3'")
def method1(self):
pass
def method2(self):
pass
def method3(self):
pass
class MyDerivedClass(MyBaseClass):
def method1(self):
print("Method1 implemented")
# This will not raise an error
d = MyDerivedClass()
class MyInvalidDerivedClass(MyBaseClass):
pass
# This will raise a TypeError
i = MyInvalidDerivedClass()TypeError: You must implement at least one of 'method1', 'method2', 'method3'Co-authored-by: Joe Hamman <[email protected]>
It's been unclear to me what methods are necessary to implement or not. I think decorating with
@abstractmethodwill help with that. It's a breaking change though and it could be disruptive.whats-new.rstapi.rst