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 abstractmethods to backend classes #7460

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Illviljan
Copy link
Contributor

@Illviljan Illviljan commented Jan 19, 2023

It's been unclear to me what methods are necessary to implement or not. I think decorating with @abstractmethod will help with that. It's a breaking change though and it could be disruptive.

  • Closes #xxxx
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@TomNicholas
Copy link
Member

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 set_variable is not defined by backends quite regularly at least.

Copy link
Member

@jhamman jhamman left a 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.

xarray/backends/common.py Outdated Show resolved Hide resolved
@@ -404,6 +411,7 @@ def __repr__(self) -> str:
txt += f"\n Learn more at {self.url}"
return txt

@abstractmethod
Copy link
Member

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.

cc @jthielen, @keewis

Copy link
Member

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 <jhamman1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants