Add motion and emg datatypes
#515
Conversation
JoeZiminski
force-pushed
the
add_motion_emg_datatypes
branch
from
304ab50 to
8573a44
Compare
for more information, see https://pre-commit.ci
|
Hey @cs7-shrey thanks a lot for this review and catching so many mistakes! Really goes to show the importance of code review, and writing tests (which I had lazily skipped 😅). I've rewritten the function as suggested and added some regression tests, let me know what you think. |
There was a problem hiding this comment.
Hey @JoeZiminski the logic is great and seems to work correctly. I have added some review comments regarding tests.
| v0.6.0 is the first version with narrow datatypes, and the checkboxes was refactored to | ||
| be a {"on": bool, "displayed": bool} dict rather than a bool indicating whether the checkbox is on. | ||
| However, this version is missing narrow datatypes added later (e.g. "motion"). | ||
| In the test file, all 'displayed' are turned off except f2pe. |
There was a problem hiding this comment.
I'm not very good at testing strategy but do you think it would be better if instead of having a hardcoded file, we had a function generate_v0_6_0_persistent_settings which would take arguments to turn checkboxes on and off in a dictionary and then write that to a yaml. Something like this
def generate_v0_6_0_persistent_settings(checkbox_type: Literal["transfer" | "create"], displayed_or_on: Literal["on", "displayed"], datatypes_turned_off: list[str]):
settings = {... the persistent_settings.yaml as a dict }
for dtype in datatypes:
settings["tui"][f"{checkbox_type}_checkboxes_on"][dtype][displayed_or_on] = False
# write settings to yaml fileWIth this we can parametrize some arguments in the tests and test a range of inputs.
Tbh, this function has a lot of moving parts and maybe it's not worth the effort to even include it and then write corresponding code in the test function. Also, the keys you've chosen to test on cover a lot of edge cases themselves so, I'll leave it to you in the end.
There was a problem hiding this comment.
Hey @cs7-shrey this is a great idea and would ideally be the best approach. In practice, it is a little fiddly because the script would have to do something like uninstall datashuttle, reinstall the version for each yaml and then build the yaml file settings in different ways depending on the datashuttle version. Because it's anticipated that we shouldn't have to do this too often, to avoid that complexity as you mention we can continue doing it manually for now. However, if we find we are continually having to manually create new yaml files, for basic stuff like adding a new narrow datatype, we should definitely take this approach. I'll resolve this for now but good to think about for the future.
| and persistent settings match the structure of the | ||
| files loaded from the old datashuttle version. | ||
| """ | ||
| project = DataShuttle("test_project") |
There was a problem hiding this comment.
This project actually exists even after the test code exits and also leaves some folders in the top level directory of the codebase. I think we could use some existing testing machinery to ensure temporary paths for "test_project"
There was a problem hiding this comment.
Good catch! I'll make a fixture for it
This PR extends the narrow datatypes to include
motionandemg, as per NeuroBlueprint PRs #67 and #68.It required extending the backwards-compatibility checking for the TUI arguments, to fill-in the settings for missing datatypes. In general, the narrow-datatype machinery is not very extensible, this should be addressed in issue #516.