Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #301 +/- ##
==========================================
+ Coverage 84.57% 84.80% +0.23%
==========================================
Files 38 39 +1
Lines 1465 1494 +29
==========================================
+ Hits 1239 1267 +28
- Misses 226 227 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
A follow-up to this will add HED tags everywhere reasonable |
|
@asmacdo Thanks for the suggestion and test, it required a little more thought to get working as intended (split into util function now) |
asmacdo
left a comment
asmacdo
left a comment
There was a problem hiding this comment.
Only strongly suggested fix is missing space nitpicks.
Optional
- Electrode hemisphere handling probably should be added?
- generalizing json and rethinking probe top level description are ideas, probably better suited for a followup if you agree.
| }, | ||
| "type": {"Description": "The type of the probe.", "LongName": "Type"}, | ||
| } | ||
| assert json_content == expected_content |
There was a problem hiding this comment.
Asserting expected_content == actual_content isn't super clear about what its testing for. This does have the advantage of showing unexpected failures more than how I originally had it, but it loses the intent. And I imagine if the output changes a developer is likely to just look at the output and if it looks fine, adjust the expected content without remembering the edge cases. Fine as-is just my 2c.
There was a problem hiding this comment.
It's a bigger pain to only have individual assertions as far as JSON dict's are concerned
True that it is more likely to break due to any minor change (such as sub content) but those are also easy to update
Co-authored-by: Austin Macdonald <austin@dartmouth.edu>
Done
Done |
|
🚀 PR was released in |
2 participants
'About time', I'm sure
Now we can have something to 'bubble up'