-
-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check if folder already exists by checking its actual name, not the name of the entry to -d flag #2425
Conversation
scripts/sct_download_data.py
Outdated
@@ -127,24 +128,46 @@ def main(args=None): | |||
data_name = arguments['-d'] | |||
verbose = int(arguments.get('-v')) | |||
sct.init_sct(log_level=verbose, update=True) # Update log level | |||
dest_folder = arguments.get('-o', os.path.abspath(os.curdir)) | |||
dest_tmp_folder = arguments.get('-o', os.path.abspath(os.curdir)+'/tmp') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"/" doesn't work in Windows, and what if user already has a local folder called "./tmp/"?
there are better ways to generate temp folders, see class sct_utils.TempFolder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, nice one, looking at it.
scripts/sct_download_data.py
Outdated
# unzip | ||
unzip(tmp_file, dest_tmp_folder, verbose) | ||
data_extracted_name = data_name | ||
if dest_tmp_folder != os.path.abspath(os.curdir)+'/bin': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why "bin"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it is the path where the installation requirements are downloaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sct_download_data
is not only used for installing binaries (look at the help)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Travis failing: https://travis-ci.org/neuropoly/spinalcordtoolbox/jobs/580121506#L1089
likely because of problem when downloading the data
scripts/sct_download_data.py
Outdated
'https://www.neuro.polymtl.ca/_media/downloads/sct/20180613_deepseg_lesion_models.zip'], | ||
'c2c3_disc_models': ['https://osf.io/t97ap/?action=download', | ||
'https://www.neuro.polymtl.ca/_media/downloads/sct/20190117_c2c3_disc_models.zip'] | ||
"sct_example_data": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmotzespina please see the convention: https:/neuropoly/spinalcordtoolbox/wiki/Single-or-double-quotes
unless you have a good reason for going with double quote?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started testing out Black, and it changed my file to double quotes, since it is kind of the black standard. I'll revert all of this changes before sending the request again.
While checking out black I also figured out that using double quotes instead of single might be a better option since we can write stuff like this: "it's a test"
otherwise, while using single quotes we would need to write the same text this way: 'it\'s a test'
.
What do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the double quote argument is indeed a good argument, but we could also make the argument that with single quote, we can do that: 'When using this option, enter parameter "n"'. Also, single quotes makes the code lighter.
Anyway, regardless the decision, this should be mixed with this PR, which is about fixing a specific problem, not changing code cosmetic. Otherwise, it makes it difficult for me to review your code.
so: open a separate issue regarding code cosmetic, and we will address it specifically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I didn't thought about the options.
Yeah, I'll revert those changes once the PR is ready, they just got mixed because of Black, but it was not intended to have those changes in this PR.
scripts/sct_download_data.py
Outdated
sct.rmtree(data_name) | ||
# Extracting the binaries doesn't require removing the previously created directories | ||
# since binaries are files, not directories | ||
if "binaries" in data_name: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will break if compressed files don't have the term "binaries" in it. Why not simply:
- check if the uncompressed zip gives you a folder
- check if the folder already exists
- if it does, remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I figured out that this would happen, I'm currently working on a fix. to have this done in a cleaner way.
scripts/sct_download_data.py
Outdated
for folder in dirs: | ||
folders.append(os.path.join(root, folder)) | ||
data_extracted_path = folders[0] | ||
directories = data_extracted_path.split("/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python has built-in modules to do such manipulations which will break with windows (when, one day, SCT will be compatible with windows...)
0e910c6
to
7914530
Compare
temp folder was already displayed by the previous line of code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
…ame_2390 Check if folder already exists by checking its actual name, not the name of the entry to -d flag Former-commit-id: 8746c6f
Removes folder names based on it's name not on
data_name
Fixes: #2425