- 
                Notifications
    
You must be signed in to change notification settings  - Fork 131
 
Fix notice_error logic for non-iterable exceptions. #1564
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
Conversation
          ✅MegaLinter analysis: Success
 See detailed reports in MegaLinter artifacts  | 
    
          Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@            Coverage Diff             @@
##             main    #1564      +/-   ##
==========================================
+ Coverage   81.76%   81.78%   +0.01%     
==========================================
  Files         207      207              
  Lines       23953    23961       +8     
  Branches     3799     3799              
==========================================
+ Hits        19585    19596      +11     
+ Misses       3101     3098       -3     
  Partials     1267     1267              ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
  | 
    
| 
               | 
          ||
| # If no exception to report, exit | ||
| if not error or None in error: | ||
| if not error or none_in_error(error) or (error and not error_is_iterable(error)): | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should only ever deal with the output of sys.exc_info, which is guaranteed to be a tuple of (None, None, None) or (exc, val, tb). This line is adding a bunch of unnecessary complexity when the original code here should have been fine.
| if not error or None in error: | ||
| # Pull from sys.exc_info() if no exception is passed | ||
| # Check that the error exists and that it is a fully populated iterable | ||
| if not error or none_in_error(error) or (error and not error_is_iterable(error)): | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the intent of this change is to make it possible to pass in an instance of BaseException to be able to report it then this change is overcomplicated and unfortunately looks to be introducing a bug.
Take for example this code:
try:
    raise ValueError()
except ValueError as exc1:
    pass
try:
    raise TypeError()
except TypeError as exc2:
    notice_error(exc1)Previously the exception instance was not allowed as an input, and would have caused an error that would crash.
With this PR's changes instead, it silently reports the wrong exception. The user is trying to pass in an exception to report, but as written this will trigger a pull from sys.exc_info() which will yield back the tuple for exc2, instead of exc1 which was passed in.
There's also a lot of complexity in these if statements now that I don't care for, as well as a bunch of new function calls that add overhead. Instead, I propose we construct a tuple in the form we expect from the exception instance if and only if the error passed in is an instance of BaseException and it has a non None __traceback__ attribute.
This reverts commit b9d9d3b.

In some cases, caught exceptions passed to notice_error() are singular class errors and not iterables like the notice_error() logic currently expects. This PR updates logic to check if the errors passed to the API exist and are fully populated iterables before grabbing
sys.exc_info()instead.