Fix LGTM recommendations

[DimitriPapadopoulos]

Description

Fixes most of the remaining LGTM recommendations: https://lgtm.com/projects/g/pydicom/deid/alerts/?severity=recommendation

Checklist

• [X] I have commented my code, particularly in hard-to-understand areas
• [x] My changes generate no new warnings
• [X] My code follows the style guidelines of this project

Open questions

The remaining recommendations should probably not be fixed.

[DimitriPapadopoulos]

About Except block handles 'BaseException':

All exception classes in Python derive from BaseException. BaseException has three important subclasses, Exception from which all errors and normal exceptions derive, KeyboardInterrupt which is raised when the user interrupts the program from the keyboard and SystemExit which is raised by the sys.exit() function to terminate the program.

Since KeyboardInterrupt and SystemExit are special they should not be grouped together with other Exception classes.

Catching BaseException, rather than its subclasses may prevent proper handling of KeyboardInterrupt or SystemExit. It is easy to catch BaseException accidentally as it is caught implicitly by an empty except: statement.

[vsoch]

@DimitriPapadopoulos a few questions / requests:

1. Could we automate this?
2. If we are replacing except with Exception, it would be ideal to catch what is actually expected there. The current change is just aesthetic.

[DimitriPapadopoulos]

Actually the current change is not aesthetic: before the change the code catches everything including BaseException, after the change it catches only subclasses of Exception. The issue raised by LGTM.com is thus fixed:

Catching BaseException, rather than its subclasses may prevent proper handling of KeyboardInterrupt or SystemExit. It is easy to catch BaseException accidentally as it is caught implicitly by an empty except: statement.

That said, I totally agree with catching only the intended exceptions, but that can be quite difficult in Python after the fact. I'd rather do that in a different PR. This PR fixes the KeyboardInterrupt/SystemExit issue.

[DimitriPapadopoulos]

I don't think it's possible to automate anything here. Tools like LGTM.com or DeepSource.io do assist in code reviewing, but we cannot let them decide blindly - yet?

[DimitriPapadopoulos]

I must have misunderstood you. I believe you want automatic LGTM reporting (not automatically fixing alerts). Of course it is possible to have a CI job that checks new PRs and raises alerts. Here is the relevant page:

Integration with GitHub Apps

[vsoch]

Thanks for the details! I will have to think about it - I'm usually pretty conservative about giving bots write access to PRs (or anything for that matter) and I'm not sure the benefits outweigh the risk. These are pretty small, trivial things.

[DimitriPapadopoulos]

Yes, many alerts are trivial, but it does happen that they hide real issues.