-
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
Fix warning due to line iterator __del__ #327
Conversation
Reviewer's Guide by SourceryThis pull request addresses a warning caused by the use of the 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: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 6 issues found
- 🟡 Complexity: 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.
Seems OK to me.
Thanks. Last one for today.... |
The fixed code caused warnings, essentially due to an antipattern. One should use context managers to clean up resources, instead of doing so in the
__del__
method. See #313 for the bigger picture.I'm planning to YOLO-merge this on Thursday, June 13 unless reviewed earlier.
Summary by Sourcery
This pull request addresses a warning related to resource cleanup by refactoring the
LineIterator
class to use a context manager instead of the__del__
method. The changes include updates to the class implementation, modifications to the usage in various parts of the codebase, and adjustments to the test cases to align with the new context manager approach.__del__
method with context manager inLineIterator
class.LineIterator
class to be used as a context manager, improving resource management and code readability.LineIterator
class as a context manager, ensuring proper resource cleanup during tests.