Template merger #105
Template merger #105
Conversation
# Conflicts: # .idea/DigLabTools.iml # .idea/misc.xml
|
Hello @sifaoufatai! Thanks for updating this PR.
Comment last updated at 2025-01-23 10:18:07 UTC |
SylvainTakerkart
left a comment
SylvainTakerkart
left a comment
There was a problem hiding this comment.
Hi! Here is my general comment: the code looks nice, but I have suggestions / questions on the naming of several classes.... Thanks to answer all the comments with corresponding commits, so that I can easily re-review after your changes!
| return data_types_data | ||
|
|
||
|
|
||
| class DataTypes: |
There was a problem hiding this comment.
why not call this class BidsDataType? this would be more explicit...
There was a problem hiding this comment.
I think it is not useful to rename it because we don't use this code anymore. We are coding through the bep32toolss repository.
There was a problem hiding this comment.
so you should remove this code!
this is why I created an issue: #108
can you please list in this issue what you think should be removed from the DigLabTools repo?
There was a problem hiding this comment.
a general comment on this file and all the others concerning the schema: the BIDS Schema is available online, I don't think it's a good idea to include it in this repo!!!
There was a problem hiding this comment.
please list this (the schema) in the issue I created about cleaning the repo!
elab_bridge/Extractor.py
Outdated
| ) | ||
|
|
||
| # Arguments for extracting | ||
| parser.add_argument("--jsonfile_extract", help="The JSON file path for extraction.", type=str) |
There was a problem hiding this comment.
same as with the Merger, there should be a version of the options with only one dash and on letter (for example: -j)
elab_bridge/Merge.py
Outdated
| ), | ||
| formatter_class=argparse.RawDescriptionHelpFormatter | ||
| ) | ||
| parser.add_argument("--sorted_list_input_files", nargs="+", help="List of JSON files to merge", type=str, |
There was a problem hiding this comment.
as I told you, there should be options with one dash and one letter, for example -s
There was a problem hiding this comment.
this can be done later... please create an issue on the repo to remember that you have to do this later!
There was a problem hiding this comment.
I'm waiting for you to create this issue before we can merge this PR (so that we don't forget!!!)
`Added short options (-letter) for long options (--words) to improve usability`
3 participants
Implement merging process for subforms and group field extraction method