FIX: Ensure values of boolean metadata fields are preserved

[tsalo]

Closes #1033. As @effigies suspected, the problem stems from the fact that metadata fields are initially converted to strings (False becomes 'False') and then are converted using the desired datatype (bool('False') becomes True).

Changes proposed:

1. Add a boolean metadata field to one of the test datasets with a value of False. I chose MTState in the 7t_trt dataset since that dataset is used in test_get_metadata.
2. Add a test, test_get_metadata_error2, that checks that the value returned is False.
3. Check for boolean datatypes in Tag._init_on_load() and explicitly convert 'False' to False. Other datatypes and values can be handled using the old code.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8fb92fa) 83.46% compared to head (db47dbd) 83.48%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1034      +/-   ##
==========================================
+ Coverage   83.46%   83.48%   +0.01%     
==========================================
  Files          38       38              
  Lines        4307     4310       +3     
  Branches     1097     1098       +1     
==========================================
+ Hits         3595     3598       +3     
  Misses        514      514              
  Partials      198      198              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tsalo]

I don't trust that this is the only place where this problem comes up, nor that this is the best way to handle it, but it gets my failing test to pass.

[tsalo]

This has me thinking that there should be a set of tests that directly compare loaded metadata values from get_metadata against values in the JSON they're loaded from. Maybe I should edit one of the datasets to have cases of each datatype we can expect for metadata fields?

[effigies]

We do have all of the types. Can we just load them as JSON and load them as metadata and compare?

bids/tests/data/7t_trt/task-rest_acq-fullbrain_bold.json

• float: EchoTime
• bool: MTState
• str: TaskName
• json: SliceTiming

bids/tests/data/7t_trt/task-rest_acq-prefrontal_physio.json

• int: StartTime

[effigies]

I think this is in good shape. I verified that my test failed without this fix and passes now.

[tsalo]

We do have all of the types. Can we just load them as JSON and load them as metadata and compare?

I think that might be a good test idea. To be honest, I might just be being paranoid. It's hard to imagine another case like this, given the other datatypes that are possible, but then again I never noticed this bug before now, so checking every possible value seems like a good idea. Still, that can be done in a separate PR.

Is there anything else I should do before merging?

[effigies]

Still, that can be done in a separate PR.

I did it in the latest commit.

I'm happy to merge if everybody else is. @adelavega?

[tsalo]

That's awesome, thanks!

Also, can we fast-track a patch release once this is merged?

[effigies]

No problem. Do you have commit rights here? Our release process is pretty easy to follow.

[tsalo]

It looks like I do have commit rights. I'm happy to take a crack at it once @adelavega weighs in on the PR.

[effigies]

Meh. I'm okay not waiting on Alej.

[tsalo]

I went through the blames for the relevant lines and I think this bug was introduced in 0.9.0 (when SQL was adopted). Does that sound right? I'd like to include the affected versions in the release notes.

[effigies]

Sounds about right.