ENH: Try to download sample data again after loading fails #876
Conversation
cpinter
force-pushed
the
redownload-corrupted-sample-datasets
branch
from
1916278
to
9313dc6
Compare
| @@ -185,7 +185,7 @@ def setup(self): | |||
| # Add spacer to layout | |||
| self.layout.addStretch(1) | |||
|
|
|||
| def logMessage(self,message): | |||
| def logMessage(self, message, isError=False): | |||
There was a problem hiding this comment.
instead of isError, you could have a logLevel parameter where value could be logging.INFO or logging.ERROR. See https://docs.python.org/2/library/logging.html#logging-levels
There was a problem hiding this comment.
I also suggest to improve self.log.insertHtml(message) so that the message is displayed in orange if logging.WARNING, and red if above that level.
| @@ -195,6 +195,10 @@ def logMessage(self,message): | |||
| self.log.insertPlainText('\n') | |||
| self.log.ensureCursorVisible() | |||
| self.log.repaint() | |||
| if isError: | |||
There was a problem hiding this comment.
replacing this with logging.log(logLevel, message) should do it then
| if os.access(filePath, os.W_OK): | ||
| os.remove(filePath) | ||
| else: | ||
| self.logMessage('<b>Load failed! Unable to delete and try again loading %s!</b>\n' % filePath, True) |
There was a problem hiding this comment.
The message is reported only if the file doesn't exist not if it couldn't be deleted.
Some exception would have to be caught.
See https://docs.python.org/2/library/os.html#os.remove
Using the Qt implementation may have a more robust behavior across platform.
cpinter
force-pushed
the
redownload-corrupted-sample-datasets
branch
from
9313dc6
to
797a66b
Compare
|
Thanks, @jcfr! I updated the commit. |
| def logMessage(self,message): | ||
| def logMessage(self, message, logLevel=logging.INFO): | ||
| # Set text color based on log level | ||
| if logLevel is logging.ERROR: |
| # Set text color based on log level | ||
| if logLevel is logging.ERROR: | ||
| message = '<font color="red">' + message + '</font>' | ||
| elif logLevel is logging.WARNING: |
| self.logMessage('<b>Load failed! Trying to download again...</b>', logging.ERROR) | ||
| file = qt.QFile(filePath) | ||
| if not file.remove(): | ||
| self.logMessage('<b>Load failed! Unable to delete and try again loading %s!</b>' % filePath, logging.ERROR) |
There was a problem hiding this comment.
If it failed to remove the file, I suggest to completely fail and not try again.
cpinter
force-pushed
the
redownload-corrupted-sample-datasets
branch
from
797a66b
to
897884f
Compare
|
@jcfr Another round done. |
|
Good to go then? |
|
Nitpick: Since the message seems to be systematically bold, you could have Assuming you tested locally, merging as it is would be perfectly fine too . |
|
I did, it works well. Good point about the bold font, I'll do that, it takes only a second. Thanks |
|
It's not consistently bold, see |
Thanks for looking into it
👍 |
|
Integrated in Slicer/Slicer@09d9918 |
Related to https://discourse.slicer.org/t/loading-of-mrhead-sample-data-set-failed/1716