STINNER Victor added the comment:
Sorry for not being more available for feedback on patches. I chose to write
the final patch because patches were not updated to take in account my latest
comments.
I hope that this issue helps you at least the process for reviewing patches ;-)
--
Roundup Robot added the comment:
New changeset 623e07ea43df by Victor Stinner in branch '3.4':
Issue #23840: tokenize.open() now closes the temporary binary file on error to
https://hg.python.org/cpython/rev/623e07ea43df
New changeset a640d268ba97 by Victor Stinner in branch '3.5':
(Merge 3.5)
STINNER Victor added the comment:
You should see the new file in the next 30 minutes.
I don't see the new file.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23840
___
STINNER Victor added the comment:
tokenizeV2.patch and tokenize.patch have issues, I reviewed them. Can someone
please write a new patch taking my comments in account?
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23840
Matt Chung added the comment:
Hey Haypo,
I'm working on submitting the new patch now. Still getting used to the
workflow and tools here. Thanks for being patient.
You should see the new file in the next 30 minutes.
--
___
Python tracker
STINNER Victor added the comment:
tokenize.patch is wrong: you should not call buffer.close() if TextIOWrapper()
was created successfully.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23840
Changes by Serhiy Storchaka storch...@gmail.com:
--
components: +Library (Lib)
stage: - needs patch
type: - resource usage
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23840
___
STINNER Victor added the comment:
I sent an email to the python core mentorship mailing list to find a volunteer
to fix this easy issue.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23840
New submission from STINNER Victor:
If TextIOWrapper constructor fails in tokenize.open(), the open binary file is
not closed and a ResourceWarning is emited.
Try attached patch:
$ python3 -Wd ~/tokenize_open_bug.py
tokenize.open failed: unknown encoding for 'test.py': xxx
Matt Chung added the comment:
Updated patch based off of haypo's comment. Changed except Exception as err:
- except
--
Added file: http://bugs.python.org/file38795/tokenize.patch
___
Python tracker rep...@bugs.python.org
shiyao.ma added the comment:
Based upon the previous review.
Catch the TextWrapper, move the test into a function.
--
Added file: http://bugs.python.org/file38796/tokenizeV2.patch
___
Python tracker rep...@bugs.python.org
Matt Chung added the comment:
Based off of storchaka's comment, moving the
--
Added file: http://bugs.python.org/file38799/tokenize.patch
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23840
Matt Chung added the comment:
I believe it goes here under the TestTokenize class. Going to give it a shot
now.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23840
___
STINNER Victor added the comment:
ITSM it's not the TextIOWrapper but the detect_encoding fails and throws an
error.
Oh, right. But TextIOWrapper can fail for differen reasons. For example, CTRL+c
may send KeyboardInterrupt.
Try for example:
with unittest.mock.patch.object(tokenize,
Matt Chung added the comment:
Comparing to introom. Any reason to not wrap the entire in a try? Attached
patch.
--
nosy: +itsmemattchung
Added file: http://bugs.python.org/file38793/issue23840.patch
___
Python tracker rep...@bugs.python.org
Matt Chung added the comment:
Curious,
@haypo, are you looking for a new unittest or a function within an existing
unit test? Perhaps go under TestTokenize?
1227 def test_main():
1228 from test import test_tokenize
1229 support.run_doctest(test_tokenize, True)
1230
shiyao.ma added the comment:
ITSM it's not the TextIOWrapper but the detect_encoding fails and throws an
error.
So I wrapped a try/catch block around that.
--
keywords: +patch
nosy: +introom
Added file: http://bugs.python.org/file38792/tokenize.patch
shiyao.ma added the comment:
On Wed, Apr 1, 2015 at 6:18 PM, STINNER Victor rep...@bugs.python.org wrote:
Oh, right. But TextIOWrapper can fail for differen reasons. For example,
CTRL+c may send KeyboardInterrupt.
Yes. Any comment on the test?
I wil update a second patch.
Regards.
--
STINNER Victor added the comment:
Yes. Any comment on the test?
Yes, as I wrote, I reviewed patches. You may get a notification by email.
Anyway, it's here:
http://bugs.python.org/review/23840/diff/14417/Lib/test/test_tokenize.py
(I also commented tokenize.py)
--
19 matches
Mail list logo
