-
Notifications
You must be signed in to change notification settings - Fork 46
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
Further improve error and warning infrastructure #349
Conversation
Reviewer's Guide by SourceryThis pull request improves the error and warning infrastructure by making all warnings and errors consistent with the previous PR that improved the 'LoadError' logic. Key changes include adding 'filename' parameters to various functions for better error messages, replacing 'warn' calls with 'LoadWarning' instances, and introducing helper functions and base classes for errors and warnings. Additionally, the '_reissue_warnings' decorator was applied to several API functions to correct the stacklevel of warnings. File-Level Changes
Tips
|
Here's the code health analysis summary for commits Analysis Summary
|
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.
Hey @tovrstra - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
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.
I just skimmed it but I think you can merge it after a final read-through, Toon.
Thanks for reviewing, Paul. I've gone through it and spotted some very minor things. I'll merge right away. |
Here is another PR corresponding to two tasks in #191, i.e. to make all warnings and errors consistent with the previous PR that improved the
LoadError
logic: #345. It was difficult to split this up in one PR per error or warning class because they make use of the same machinery.Main improvements:
LineIterator
methods for errors and warnings are gone.stacklevel
. (For more details, see https://docs.python.org/3/library/warnings.html#warnings.warn and https://stackoverflow.com/a/71635963/494584)Related points deferred to later pull requests:
dump_*
sections inCONTRIBUTING.md
. This will be done after theprepare_dump
API has been completed.I will YOLO-merge this on Friday, June 28, unless reviewed earlier.
Summary by Sourcery
This pull request improves the error and warning infrastructure by standardizing messages to include file and line number information, introducing base classes for file-related errors and warnings, and refactoring functions to handle warnings more effectively. It also updates the
prepare_dump
function signatures and adds tests for the new warning behaviors.prepare_dump
function signatures to include the filename parameter for better error context.LineIterator
class to remove thewarn
method and handle warnings directly using thewarn
function.