-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
remove overlapping slots from app.Sanic, fix broken slots inherit of HTTPResponse #2387
Conversation
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.
Changes lgtm
Codecov Report
@@ Coverage Diff @@
## main #2387 +/- ##
=============================================
+ Coverage 87.107% 87.110% +0.002%
=============================================
Files 60 60
Lines 5003 5004 +1
Branches 905 905
=============================================
+ Hits 4358 4359 +1
Misses 472 472
Partials 173 173
Continue to review full report at Codecov.
|
Thanks. Can you add that to the GH actions pipeline? |
@prryplatypus does the one test failure look at all familiar to you? In theory my change could have affected the cookies in |
Agreed. I've never seen that test be flappy, so it is suspicious, but I wouldn't worry about that yet. I'll take a look and see what's going on with it. Thanks for your effort in reaching out and putting this together. We actually have a lot more slots usage coming around the corner in some refactors, and with all the nesting I appreciate your utility. |
@ahopkins awesome! Let me know if you have any ideas for features/improvements. |
Summary
There are some
__slots__
-related problems insanic
:HTTPResponse
won't get a__dict__
and saves a bit of memory in the process.Questions
regarding tests: probably not needed. I discovered the slots issue with slotscheck, a tool I maintain. Of course, If you like, I can add it to tox/CI as I've done for instagram/LibCST and dry-python/returns.