Add docstrings and expand tests for IO sub-module

brainglobe / brainglobe-utils

Shared general purpose tools for the BrainGlobe project

MIT License

11 stars 1 forks source link

Add docstrings and expand tests for IO sub-module #65

Closed K-Meech closed 6 months ago

K-Meech commented 6 months ago

Description

What is this PR

[ ] Bug fix
[ ] Addition of a new feature
[X] Other

Why is this PR needed? There are very few docstrings for functions in the IO sub-module. Also some parts aren't covered by tests.

What does this PR do? This PR adds docstrings for all functions in the IO sub-module. It also expands/refactors the tests in test_cell_io

References

For https://github.com/brainglobe/brainglobe-utils/issues/19

How has this PR been tested?

All tests pass locally.

Is this a breaking change?

Does this PR require an update to the documentation?

Checklist:

[X] The code has been tested locally
[X] Tests have been added to cover all new functionality (unit & integration)
[X] The documentation has been updated to reflect any changes
[X] The code has been formatted with pre-commit

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.48%. Comparing base (cacefd6) to head (333e05a). Report is 7 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #65 +/- ## ========================================== + Coverage 86.49% 90.48% +3.99% ========================================== Files 35 35 Lines 1355 1356 +1 ========================================== + Hits 1172 1227 +55 + Misses 183 129 -54 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

K-Meech commented 6 months ago

@willGraham01 I've looked through brainglobe_utils and turns out there are many files that use pathlib.Path rather than Path. Are you happy for me to merge this PR as-is, then make a separate PR to fix all the Path issues at once? Otherwise, it will introduce changes across files outside the IO sub-module.

willGraham01 commented 6 months ago

Are you happy for me to merge this PR as-is, then make a separate PR to fix all the Path issues at once? Otherwise, it will introduce changes across files outside the IO sub-module.

Yep happy for this to go in as it is. I'll open a PR of my own to change all the pathlib.Path syntax across the package since that's become a pet peeve of mine :sweat_smile: