catalystneuro / nwb-conversion-tools

Create NWB files by converting and combining neural data in proprietary formats and adding essential metadata.
https://nwb-conversion-tools.readthedocs.io/en/main/
BSD 3-Clause "New" or "Revised" License
25 stars 12 forks source link

interface schemas as class properties #161

Closed CodyCBakerPhD closed 3 years ago

CodyCBakerPhD commented 3 years ago

sneakers-the-rat's fork of the repo diverges in a number of ways. One such way is to specify schema objects as properties inside the NWBConverter, as containers for the get_schema functions. This sets them as internal attributes of the class in a pythonic way following get/set methods.

Is your feature request related to a problem? Please describe. Instead of the user directly calling the methods of, for example, my_nwb_converter.get_source_schema() in order to view the contents, they would try to access my_nwb_converter.source_schema.

Describe the solution you'd like The basic code is

    @property
    def source_schema(self):
        return self.get_source_schema()

and would be set as such for each of the other schemas. Note that the first stage of the @property decorator is the 'get' method, where here is simply wrapping the function that assembles and returns the schema. We could also break this into the more formal two-step of

    @property
    def source_schema(self):
        return self._source_schema

    @property.setter
    def source_schema(self):
        self._source_schema = self.get_source_schema()

Describe alternatives you've considered There are some conveniences this approach can offer to backcompatability/refactoring, but in the end we still do need to be careful when renaming internal attributes and anything that depends on them, so this might just take less code? But we also don't treat them as internal attributes at the moment, anyway, so what we offer right now is the most basic functionality to get the job done, which is the function and only the function to retrieve and assemble the schemas for a given step. Keep in mind that adding more features, such as adding the schemas as an attribute, is one more thing we need to track and maintain for supporting the repo.

@luiztauffer Thoughts?

Additional context Read more about properties here: https://www.programiz.com/python-programming/property

Checklist

CodyCBakerPhD commented 3 years ago

Team discussed, and while this does offer some small intuitive use and small computational improvement, we just don't think it's impactful enough to take action on.