-
Notifications
You must be signed in to change notification settings - Fork 335
Remove __del__ from Redis (Fixes #1115) #1227
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1227 +/- ##
==========================================
- Coverage 90.67% 90.66% -0.01%
==========================================
Files 21 21
Lines 6875 6869 -6
Branches 794 793 -1
==========================================
- Hits 6234 6228 -6
Misses 469 469
Partials 172 172
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I'm fine with this, but better would be to copy the code in aiohttp to raise a warning: |
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 don't have anything to add to what @Dreamsorcerer has said.
* Fixed getting proper event loop
@bmerry @seandstewart sorry for the spam requests for review. After abrookins left, I'm wondering who still currently has power for approving PRs to be merged? |
@Andrew-Chen-Wang - I believe you should have power to merge this. LGTM 🚢 |
Thanks @seandstewart . To clarify, PRs require authorized reviewers to approve PRs before they get merged. Just wondering besides us two, who else has the privilege to approve PRs since I feel guilty spam requesting reviews 😅 |
What do these changes do?
Removes
__del__
fromRedis
Are there changes in behavior for the user?
Yes, you should explicitly disconnect and not use
__del__
Related issue number
Fixes #1115
Checklist
CONTRIBUTORS.txt
<Name> <Surname>
.CHANGES/
folder<issue_id>.<type>
(e.g.588.bugfix
)issue_id
change it to the pr id after creating the PR.feature
: Signifying a new feature..bugfix
: Signifying a bug fix..doc
: Signifying a documentation improvement..removal
: Signifying a deprecation or removal of public API..misc
: A ticket has been closed, but it is not of interest to users.Fix issue with non-ascii contents in doctest text files.