-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
raise ResourceWarnings as errors #2842
Conversation
fdefe53
to
5ab8ebc
Compare
6ff1be2
to
4d425a2
Compare
d80e799
to
95cf058
Compare
95cf058
to
c575ddd
Compare
a5d4269
to
c744f19
Compare
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'm happy with this PR in its current state, @graingert did you want this merged before all ResourceWarning
s are fixed or are you okay with it happening whenever?
You may need to add the resource warning skip to the Emscripten tests as well, unless the issue is fixable on our side?
This reverts commit 328bda5.
d5e7b8b
to
328bda5
Compare
@sethmlarson looks like it's fixable on the urllib3 side, but I raised it here just in case pallets/quart#303 |
@sethmlarson it would be great to get this merged, I think #3240 is the last bit that needs merging first as that was issuing a ResourceWarning |
@sethmlarson I think this is ready to go now? |
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.
LGTM, thanks so much @graingert!
fixes #2970