BF (TST): make anonymize_script actually output anything and map determinstically #511
Conversation
…ically hash() would get a new value for every run for the same sid, so not a good choice. Without print script produced no output so code just saw empty string and assumed that no annonymization was needed etc
Codecov Report
@@ Coverage Diff @@
## master #511 +/- ##
==========================================
+ Coverage 77.04% 77.55% +0.51%
==========================================
Files 41 41
Lines 3123 3154 +31
==========================================
+ Hits 2406 2446 +40
+ Misses 717 708 -9
Continue to review full report at Codecov.
|
…ed output So it will be for the script to just return original sid if it decides to not anonymize it, which is to some degree change in behavior. But I think it would be beneficial in the long run due to more straight-forward and robust operation. Docstring for anon_cmd does not mention that empty output implies no change in subject id
The problem was that the glob expressions were off and no files matched in either case and then empty lists matched just fine. I have fixed up the test so it - does verify that anonimization changed the subject id if anonimization script was provided, and does not if it was not - also converted to use f""strings instead of .format
anonymization script printed nothing, which then taken as empty sid, and thus main code just proceeded without any anonymization. In 74550b8 have made the In subsequent commit 1d6b41b I have fixed the test -- it was just comparing "nothing" to be equal "nothing", and thus not useful and passing. |
|
since it is relating only to the tests, I think it is not worth holding on merging. I will attend to comments if made after |
|
🚀 PR was released in |
hash() would get a new value for every run for the same sid, so not a good
choice. Without print script produced no output so code just saw empty
string and assumed that no annonymization was needed etc
was looking into the anonymization logic for #509 to realize that this script does nothing... fixed it but it seems that test is still working as expected which imho it shouldn't
TODOs