Improve specs and enforce consistency of container class docstring and default description

agencyenterprise / ndx-nirs

An NWB extension for storing Near-Infrared Spectroscopy (NIRS) data

BSD 3-Clause "New" or "Revised" License

5 stars 2 forks source link

Improve specs and enforce consistency of container class docstring and default description #8

Closed bendichter closed 2 years ago

bendichter commented 3 years ago

Depends on upcoming release of HDMF

dsleiter commented 2 years ago

@bendichter I tried to push the fixes I mentioned, but it was denied. Could you please select "Allow edits from maintainers"?

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Let me know if you don't have the checkbox available as shown in that link, you might be running into the same issue I have when contributing to pynwb from a fork owned by an organization. I have in my notes somewhere how to use the github webapi as a workaround to set that field.

bendichter commented 2 years ago

@dsleiter those adjustments sound right.

For the matter of if there are advantages to doing it this way, the only real advantage is having less code and therefor less room for error. I generally encourage new users to do it automatically, because the manual way can require a lot of background knowledge about docval and how NWB works internally. However, specifying it manually has the advantage that it allows you to make adjustments in the mapping between the API and the neurodata type. I like having this relationship be deterministic and predictable, but that may become a problem, particularly if you want to change the schema while keeping the API the same. The automatic way does have the ability to do some custom input validation, as demonstrated here.

bendichter commented 2 years ago

I have added you as a contributor to my branch of the extension. If that doesn't work, another approach might be to pull from my branch and edit it yourself in your own fork

dsleiter commented 2 years ago

@bendichter thanks again for the PR. We decided not to use auto-class generation for the dynamic tables in order to continue using the customized class behavior without needing a bunch of monkey patching. We do continue using the auto-generation for other types though.

You also made some improvements to the specs, so we want to keep those. Based on our discussion above, I also added some tests to verify consistency between the spec and the container class in the following ways:

the class docstring should match or at least start with the value of the 'doc' field in the neurodatatype spec.
the default value for the description in the class initializer should match the 'default_value' field of the description attribute in the spec

If you have the time and desire, could you please take a look at the updates to the PR?