acsresearch / interlab

MIT License
17 stars 3 forks source link

Storage interface cleanup #35

Closed jas-ho closed 11 months ago

jas-ho commented 11 months ago

disclaimer: this is probably a much smaller issue than it sounds

background: I tried to implement my own storage class and was a bit confused by the interface. Part of it is lack of documentation (I've added this to #32 ). But I think it might be somewhat simplified still.

For example both read_root and display seem to be unused.

jas-ho commented 11 months ago

stylistic question: why not declare it an abstract class and use @abstractmethod decorators instead of explicitly raising NotImplementedErrors? I think this style is both more conventional and more convenient.

For example, when I explore a Python class interface, the first thing I do is fold all methods (using ctrl+shift+minus). If people use @abstractmethod decorators the things I need to implement stick out visually immediately. image

Also tooling relies on this style. For example, in Pycharm, I can inherit an abstract class and it automatically offers to add stubs for all methods which are abstract in the interface. That does not work if you don't follow the @abstractmethod style

spirali commented 11 months ago

I will improve the doc and code style, I have nothing against @abstractmethod but Pycharm offers me an implementation of these methods even without this decorator. display() is used in Jupyter notebooks.

In general, "root" methods are used for the left panel in the browser. If you have any particular question about interface let me know.

jas-ho commented 11 months ago

thanks for the comments @spirali, that's all good to know. Given this information, I'm happy if you close the issue if you feel it's not relevant.

spirali commented 11 months ago

I will let it open to remind me to improve and document the API.