search

INT-NIT / BEP032tools

Software tools supporting the BIDS Extension Proposal (BEP) dedicated to adding support for electrophysiological data recorded in animal models (BEP032)
MIT License
3 stars 9 forks source link

Clean up of recently merged version #151

Open SylvainTakerkart opened 3 months ago

SylvainTakerkart commented 3 months ago

Hello @sifaoufatai ,

Before implementing new functionnalities, I think it would be good to clean up the current version of your code and do some refactoring...

  1. the bep32v01 directory needs to be renamed because:

    • you should not have a version number in the directory name
    • it's not related to bep32 anymore (electrophy) but it is aimed at being more generic... so you can call it BIDSTools (note that we should also rename the name of the full project, but let's ignore this for now, let's just deal with your code / directory!)

  2. about launch.py

    • it should be renamed into something else... for example CreateBIDSDataset.py, or something else; please discuss here and let's converge in this discussion before you implement!
    • translate everything from french to english in your code! (variable names, comments etc.)
    • rename the variables config_path and output_path: one is a file, the other one is a directory, so they should not be called with similar names (with both path)... to make the difference between files and directories, you can look at other big open source projects and see how they call files and directories (for ex. scikit learn)
    • simplify the code... lines like json_path = config_path or output = output_path should not exist!: use only one variable name, a meaningful one ;)

@killianrochet can check on all these details if I'm not available tomorrow!

killianrochet commented 3 months ago

Hello @sifaoufatai ,

I agree with all your comments, and I just want to add something that's very important in my opinion:

What I mean is that you don't have to use / test the functions in the main files, you have to test / use them in the test_files which are provided for that purpose. Later on you'll start doing unit tests, so I think it's a good idea to start adopting this separation to simplify your life in the future.