-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 ResourceWarnings in tests #1660
Conversation
1120982
to
e2ff841
Compare
@@ -135,7 +134,6 @@ def __exit__(self, type, value, traceback): | |||
nocm = NoCM() | |||
|
|||
|
|||
@contextmanager | |||
def file_or_filename(input): |
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.
As I see in usage, this function used as contextmanager, why you remove this (@contextmanager
+ yield
) ?
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.
Oh, it really doesn't work as was expected, nice catch!
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.
After meged comment:
Actually, it was already brokenl. Compare with code:
from contextlib import contextmanager
@contextmanager
def g():
print("g input")
yield
print("g exit")
@contextmanager
def f():
print("f input")
yield g()
print("f exit")
with f():
print("with block")
This code prints:
f input
with block
f exit
But expected something like:
f input
g input
with block
g exit
f exit
Back to the file_or_filename
, methods __enter__
and __exit__
of smart_open(input)
or input
are not called at all. Oops.
@@ -895,8 +895,11 @@ def docbyoffset(self, offset): | |||
assert previd <= docid, "matrix columns must come in ascending order" | |||
if docid != previd: | |||
if previd >= 0: | |||
return document | |||
break |
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.
Can you add a test for this?
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.
Hmm, ok. But I don't understand what exactly should I check in it ?
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 read full method definition, now it's clear for me, thanks.
No description provided.