-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
Fix LGTM recommendations #191
Fix LGTM recommendations #191
Conversation
Except block directly handles BaseException.
Import of 'remove_sequences' is not used.
The value assigned to local variable 'padded_expected_length' is never used.
About Except block handles 'BaseException':
|
@DimitriPapadopoulos a few questions / requests:
|
Actually the current change is not aesthetic: before the change the code catches everything including
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 |
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? |
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: |
Yes, many alerts are trivial, but it does happen that they hide real issues. |
Description
Fixes most of the remaining LGTM recommendations:
https://lgtm.com/projects/g/pydicom/deid/alerts/?severity=recommendation
Checklist
Open questions
The remaining recommendations should probably not be fixed.