BUG: fix runtime error related to creation of DICOM database directory #640
Conversation
| try: | ||
| os.mkdir(databaseDirectory) | ||
| except OSError: | ||
| databaseDirectory = defaultDatabaseDirectory |
There was a problem hiding this comment.
@fedorov I would assign defaultDatabaseDirectory=os.path.join(documents, "SlicerDICOMDatabase") here because we could run into problems causing the variable to be referenced before assigned
There was a problem hiding this comment.
You mean it could happen with testingEnabled, but failing to create the directory? I guess it is not impossible, but in this case wouldn't it make sense to move definition of defaultDatabaseDirectory to the top of the function?
There was a problem hiding this comment.
Oh yeah makes sense. I didn't see the message variable...
|
LGTM |
| if not os.path.exists(databaseDirectory): | ||
| os.mkdir(databaseDirectory) | ||
| try: | ||
| os.mkdir(databaseDirectory) |
There was a problem hiding this comment.
os.makedirs works recursively, use that to take care of nested directories
(permission or other issues may still prevent directory creation, so it's fine to keep exception handling)
There was a problem hiding this comment.
@pieper is there any particular reason why os.makedirs was not used in the first place? If there is no some hidden logic, I will push the change.
| message = "DICOM Database will be stored in\n\n{}\n\nUse the Local Database button in " \ | ||
| "the DICOM Browser to pick a different location.".format(databaseDirectory) | ||
| slicer.util.infoDisplay(message, windowTitle='DICOM') | ||
| documentsLocation = qt.QDesktopServices.DocumentsLocation |
There was a problem hiding this comment.
I had to spend several minutes reviewing this couple of lines of code and I'm still not sure about intent and correctness. Please put variable initializations where to the closest possible place where variables are actually used (similarly as it was in the original version).
There was a problem hiding this comment.
I did not move initialization of the variables, they remain in the same places where they were in the original version. It looks like I moved them, but I just changed the spacing. I think the location is alright, not sure what can be improved here.
There was a problem hiding this comment.
It might be easier to look at the actual function, not the change set: https://github.com/fedorov/Slicer/blob/3da900772d24cbf809725576de4bfceaf9bdffe5/Modules/Scripted/DICOMLib/DICOMWidgets.py#L489-L510
|
I updated the code to rearrange few variables and use os.makedirs(). |
|
@lassoan I believe I addressed your concerns. Please let me know if any further changes are needed. Thank you. |
|
Thank you. |
In some situations, user settings may point to a temporary directory with multiple nesting levels, which cannot be created by os.mkdir(). To solve the issue, when exception occurs, fall back to creating the DICOM DB directory in the default location. Also fix a typo for the name of the DICOM DB location in developer mode.
Also use os.makedirs() in place of os.mkdir()
fedorov
force-pushed
the
fix-dicom-dir-creation-bug
branch
from
dd3a7e3
to
fc2bd1c
Compare
|
Thanks! Integrated in r25633 = 2a6e43e |
In some situations, user settings may point to a temporary directory with multiple
nesting levels, which cannot be created by os.mkdir().
To solve the issue, when exception occurs, fall back to creating the DICOM DB
directory in the default location.
Also fix a typo for the name of the DICOM DB location in developer mode.
cc: @che85