Skip to content

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.

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'

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