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

Specify interfaces for inheritance with abstract base classes #26

Open
jlvdb opened this issue Jul 26, 2023 · 2 comments
Open

Specify interfaces for inheritance with abstract base classes #26

jlvdb opened this issue Jul 26, 2023 · 2 comments
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation help wanted Extra attention is needed

Comments

@jlvdb
Copy link

jlvdb commented Jul 26, 2023

Some base classes in RAIL are commonly used implement algorithms or other utilities. In some cases it is not immediately clear which class methods are part of the base implementation and which should be overwritten/implemented by a subclass.

Example

rail.core.DataHandle: The methods _open, _read, _write, _initialize_write, _write_chunk, _finalize_write, _size, _iterator all raise NotImplementedErrors when the subclass does not implement them. ModelHandle for example implements only the first three methods, which may lead to unexpected behaviour. It is not clear which methods are required and in which context exceptions may occur. Furthermore, the methods are not fully documented and it is not always clear which functionality and in/outputs must be covered.

Suggestions

  • Description of the expected behaviour in the doc-string and implementation of base classes with python's abc.ABC, and methods with the @abstract... decorators. Missing method implementations will be flagged when creating a class instance instead of during runtime.
  • If some methods are only required in certain context, create a new subclass for theses situations and move the methods there, e.g.:
class ChunkedDataHandle(DataHandle):
    @abstractmethod
    def _initialize_write(self): ...

    @abstractmethod
    def _write_chunk(self): ...

and remove these methods from DataHandle.

Impact

Simplifies subclassing and clarifies the logical relationship between different classes.

@jlvdb jlvdb added documentation Improvements or additions to documentation enhancement New feature or request labels Jul 26, 2023
@aimalz aimalz added bug Something isn't working help wanted Extra attention is needed and removed enhancement New feature or request labels Jul 27, 2023
@jlvdb jlvdb self-assigned this Aug 3, 2023
@jlvdb
Copy link
Author

jlvdb commented Aug 3, 2023

I will start by adding the missing doc strings in core.data

@jlvdb
Copy link
Author

jlvdb commented Aug 3, 2023

Next add a subclassing guide to the class docstring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants