-
Notifications
You must be signed in to change notification settings - Fork 23
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
Upgrade to Pydantic 2 #767
Conversation
current_location=neurodata_object.name + "/" + current_location, neurodata_object=parent | ||
) | ||
|
||
|
||
def _find_location_in_memory_nwbfile(neurodata_object: Container, field_name: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor readability change to the private methods; otherwise below there was a line that read as _find_location_in_memory_nwbfile(current_location=field_name)
which looked weird until you followed the redirection and implicit logic enough to realize how the location is constructed recursively
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got you, this makes the signature more meaningful. Glad we make all of this private.
Should't the typing be NWBContainer? https://pynwb.readthedocs.io/en/stable/pynwb.core.html#pynwb.core.NWBContainer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an NWBFile can have something that's a Container
but not an NWBContainer
? Maybe, maybe not - the typing and logic should apply to the most generic case however
src/neuroconv/tools/nwb_helpers/_configuration_models/_base_dataset_io.py
Show resolved
Hide resolved
src/neuroconv/tools/nwb_helpers/_configuration_models/_base_dataset_io.py
Show resolved
Hide resolved
src/neuroconv/tools/nwb_helpers/_configuration_models/_base_dataset_io.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems OK to me.
It is flattening the structure that we use to write the backend plus some minor changes that I agree with: field for dataset_name, creating another function for finding the location which has better signature semantics and making some properties class variables.
The most difficult thing is to read carefully through the test as the flattening of the structure implied that they have to change.
I also think that the diff in the tests resulted a bit larger than it had to be because we are testing string representations.
I will re-read the tests tomorrow just as a sanity check for my eyes and approve after as I think this is ready to go.
current_location=neurodata_object.name + "/" + current_location, neurodata_object=parent | ||
) | ||
|
||
|
||
def _find_location_in_memory_nwbfile(neurodata_object: Container, field_name: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got you, this makes the signature more meaningful. Glad we make all of this private.
Should't the typing be NWBContainer? https://pynwb.readthedocs.io/en/stable/pynwb.core.html#pynwb.core.NWBContainer
src/neuroconv/tools/nwb_helpers/_configuration_models/_base_dataset_io.py
Show resolved
Hide resolved
src/neuroconv/tools/nwb_helpers/_configuration_models/_base_dataset_io.py
Show resolved
Hide resolved
src/neuroconv/tools/nwb_helpers/_configuration_models/_base_dataset_io.py
Show resolved
Hide resolved
src/neuroconv/tools/nwb_helpers/_configuration_models/_base_dataset_io.py
Show resolved
Hide resolved
src/neuroconv/tools/nwb_helpers/_configuration_models/_base_backend.py
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #767 +/- ##
==========================================
- Coverage 91.17% 91.08% -0.10%
==========================================
Files 121 121
Lines 6315 6296 -19
==========================================
- Hits 5758 5735 -23
- Misses 557 561 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests look OK to me. This is good to go.
) | ||
|
||
|
||
@pytest.mark.parametrize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that this is done by pydantic now.
...backend_and_dataset_configuration/test_helpers/test_get_default_dataset_io_configurations.py
Show resolved
Hide resolved
HDF5BackendConfiguration, | ||
HDF5DatasetIOConfiguration, | ||
ZarrBackendConfiguration, | ||
ZarrDatasetIOConfiguration, | ||
) | ||
|
||
|
||
def mock_DatasetInfo( | ||
def mock_HDF5DatasetIOConfiguration( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mock at the beginning of the name. I think a good idea. It could also be mock_DatasetIOConfigurarion(backend: ["hdf5", "zarr"])) to save some code duplcation.
But I am not sure that is better and probably does not really matter for only two objects.
DANDI just finished the upgrade to Pydantic 2, so we need to keep up for our live service automatic upload + GUIDE bundling
Also fixes #662 by using
typing_extensions.Self