Pylint Exception logging #9077
Pylint Exception logging #9077
Conversation
Merge azure-sdk-tools main into main
Update working-main
More testing still to come
Also added more test cases
Also added one test and link for python implementation
…exception-logging
Edits to fix false positives from testing against SDKs. Added more unit tests
Fix another false positive from the SDKs
Added corresponding test
Fixed another false positive
…exception-logging
| expression = j.as_string().lower() | ||
| if any(x in expression for x in levels_matches) and "logger" in expression: | ||
| # Check for variables after strings | ||
| end_finder = expression.rfind("'") |
There was a problem hiding this comment.
could you explain an example where the end_finder would be used? Would it always be a single quote string?
There was a problem hiding this comment.
This was added to help fix a false positive from testing against the SDKs. end_finder finds the end of any strings being logged which should give access to just the variables after. For the code example below, this should mean that the checker is triggered for the variable 'exception' rather than the usage of the word 'exception' in the string.
except Exception as exception:
logger.warning("There has been an exception", exception)
If there is only a string and no variables after, it should look at the string instead.
As for the single quote, when the checker looks at the string it seems to convert double quotes to single quotes. Think the culprit might be expression = j.as_string(). So while the above code uses double quotes, when my checker runs they switch to single. Happy to investigate this further if you'd like.
| for i in range(len(expression1)): | ||
| if exception_name == expression1[i]: | ||
| if i+1 < len(expression1): | ||
| if "." and "name" not in expression1[i+1]: |
There was a problem hiding this comment.
Is this looking for if exception.__name__ is used? Are there any other cases here where we would not want to raise a pylint error? (if not sure we could add a #TODO comment for future investigating)
There was a problem hiding this comment.
Yes, this is looking for exception.__name__. Could make it more specific - e.g. change if "." and "name" not in expression1[i+1]: to if ".__name__" not in expression1[i+1]: if you think that would be better.
As for other cases, didn't find any while testing against the SDKs, but can look into it further.
.../pylint-extensions/azure-pylint-guidelines-checker/tests/test_files/do_not_log_exceptions.py
Show resolved
Hide resolved
…on_verify Do not hardcode boolean connection verify
Invalid use of @overload
Merge main into exception-logging
Address merge conflicts and finalize merge.
Addresses issue #3227