hdmf icon indicating copy to clipboard operation
hdmf copied to clipboard

Write dimension labels to DatasetBuilder on build

Open rly opened this issue 1 year ago • 1 comments

Motivation

Fix #1077

How to test the behavior?

pytest

Checklist

  • [x] Did you update CHANGELOG.md with your changes?
  • [x] Does the PR clearly describe the problem and the solution?
  • [x] Have you reviewed our Contributing Guide?
  • [x] Does the PR use "Fix #XXX" notation to tell GitHub to close the relevant issue numbered XXX when the PR is merged?

rly avatar Mar 25 '24 18:03 rly

Codecov Report

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

Project coverage is 88.89%. Comparing base (77be0cc) to head (5446b60). Report is 26 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1081      +/-   ##
==========================================
+ Coverage   88.83%   88.89%   +0.05%     
==========================================
  Files          45       45              
  Lines        9784     9833      +49     
  Branches     2779     2794      +15     
==========================================
+ Hits         8692     8741      +49     
  Misses        776      776              
  Partials      316      316              

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

codecov[bot] avatar Mar 25 '24 18:03 codecov[bot]

@rly can you move BuildDatasetShapeMixin to a more general location? When importing this mixin class, it would be cleaner to have it a test_utils.py or test_mixin.py.

Otherwise, looks good.

mavaylon1 avatar Jul 24 '24 18:07 mavaylon1

@mavaylon1 The place to move it would be https://github.com/hdmf-dev/hdmf/blob/dev/tests/unit/helpers/utils.py. However, moving that means also moving the container classes and mappers defined in this file tests/unit/build_tests/mapper_tests/test_build.py. We might then want to resolve any redundancies between the Bar, Foo, and Baz test classes. I can do that, but I think this task fits well under the test harness refactoring that you wanted to do. By keeping it here for now, it's clear that this mixin is used only in this file. Should I move it, or let you take care of this as part of the larger refactoring?

rly avatar Jul 24 '24 23:07 rly

TODO: I will look into the coverage failure

rly avatar Jul 25 '24 21:07 rly