Fix LGTM alerts #186
Fix LGTM alerts #186
Conversation
| if os.path.isfile(os.path.join(tempdir, f)) | ||
| ) | ||
|
|
||
| if dicom_file is not None: |
There was a problem hiding this comment.
Looks like you forgot to add back the check here.
There was a problem hiding this comment.
Not at all. It's useless. See commments in commit.
There was a problem hiding this comment.
@BrianKolowitz it looks like you added this check - can you confirm the change looks okay to you?
There was a problem hiding this comment.
Maybe it wasn't useless before, now it is useless because of the additional try/except.
There was a problem hiding this comment.
dicom_file cannot be empty because we continue in case of StopIteration - or an exception is raised in case of other erros.
There was a problem hiding this comment.
Understood, and it LGTM - I'm just hoping to get the blessing of the original contributor before changing the code he wrote.
There was a problem hiding this comment.
Sure, I'd like to have it too.
There was a problem hiding this comment.
@wetzelj it looks like @BrianKolowitz is not able to respond (I hope he's okay!) and I'd like to get one more review on this before merge. It's fairly small changes - if you have time in the next few weeks could you take a look?
There was a problem hiding this comment.
Sorry for my delayed response... I was out of the office last week and have been playing catch up.
Looking at this change, it looks like it should be okay. Since the next() will raise StopIteration when the iterator is exhausted (i.e. there are no items left in the zipfile), the continue statement that you've put in on StopIteration will continue to the next item up for processing in the list loop on line 60 (iterating through multiple files/directories).
There was a problem hiding this comment.
Thanks @wetzelj ! That is my sentiment as well.
Call to next() in a generator Additionnlly, dicom_file cannot be None: • os.path.splitext(dicom_file) would raise TypeException • In the case of a ZIP file, which I believe is the one the existing code was trying to address, we continue on StopIteration if the ZIP file is empty.
Import of 'Sequence' is not used. Import of 'remove_sequences' is not used. Import of 'os' is not used. Import of 'read_json' is not used. Import of 'DeidRecipe' is not used. Import of 'get_private' is not used. Import of 'remove_sequences' is not used. Import of 'extract_fields_list' is not used. Import of 'extract_values_list' is not used. Import of 'get_fields' is not used. Import of 'DataElement' is not used. Import of 'operator' is not used. Import of 'get_installdir' is not used. Import of 'os' is not used. mport of 'read_file' is not used. Import of 'os' is not used.
Except block directly handles BaseException. Function dateutil.parser.parse() raises the following exceptions: • ParserError – Raised for invalid or unknown string formats, if the provided tzinfo is not in a valid format, or if an invalid date would be created. • OverflowError – Raised if the parsed date exceeds the largest valid C integer on your system.
Description
Fixes a large part of these LGTM alerts:
https://lgtm.com/projects/g/pydicom/deid/
Checklist