Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(4997)

Issue 52075: [issue5883] detach() implementation (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 11 months ago by Antoine Pitrou
Modified:
14 years, 8 months ago
Reviewers:
CC:
report_bugs.python.org
Base URL:
http://svn.python.org/view/*checkout*/python/branches/py3k/
Visibility:
Public.

Patch Set 1 #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+180 lines, -9 lines) Patch
Doc/library/io.rst View 3 chunks +17 lines, -1 line 2 comments Download
Lib/_pyio.py View 5 chunks +33 lines, -0 lines 1 comment Download
Lib/test/test_io.py View 4 chunks +29 lines, -0 lines 2 comments Download
Modules/_io/bufferedio.c View 11 chunks +53 lines, -4 lines 2 comments Download
Modules/_io/textio.c View 6 chunks +48 lines, -4 lines 3 comments Download

Messages

Total messages: 2
Antoine Pitrou
14 years, 11 months ago (2009-04-30 15:51:35 UTC) #1
Antoine Pitrou
14 years, 11 months ago (2009-04-30 16:04:45 UTC) #2
http://codereview.appspot.com/52075/diff/1/2
File Doc/library/io.rst (right):

http://codereview.appspot.com/52075/diff/1/2#newcode366
Line 366: Disconnect this buffer from its underlying raw stream and return it.
This sentence is a bit ambiguous.
How about “Separate the underlying raw stream from the :class:`BufferedIOBase`
and return it” ?

Also, you should mention that some implementations will raise
io.UnsupportedOperation if the operation doesn't make sense.

http://codereview.appspot.com/52075/diff/1/2#newcode605
Line 605: in an unusable state.
You should mention that some implementations will raise io.UnsupportedOperation
if the operation doesn't make sense.

http://codereview.appspot.com/52075/diff/1/3
File Lib/_pyio.py (right):

http://codereview.appspot.com/52075/diff/1/3#newcode836
Line 836: return self
Uh, this doesn't really make sense. Better let it raise io.UnsupportedOperation.

http://codereview.appspot.com/52075/diff/1/4
File Lib/test/test_io.py (right):

http://codereview.appspot.com/52075/diff/1/4#newcode532
Line 532: self.assertIs(buf.detach(), raw)
Perhaps a test of what happens when calling detach() a second time?
(it should probably raise a ValueError)

http://codereview.appspot.com/52075/diff/1/4#newcode1504
Line 1504: self.assertIs(t.detach(), b)
Same comment as for buffered binary tests.

http://codereview.appspot.com/52075/diff/1/5
File Modules/_io/bufferedio.c (right):

http://codereview.appspot.com/52075/diff/1/5#newcode470
Line 470: Py_CLEAR(self->raw);
Why not the simpler:
    raw = self->raw;
    self->raw = NULL;
?

http://codereview.appspot.com/52075/diff/1/5#newcode1547
Line 1547: self->detached = 1;
This should be 0.

http://codereview.appspot.com/52075/diff/1/6
File Modules/_io/textio.c (right):

http://codereview.appspot.com/52075/diff/1/6#newcode1080
Line 1080: "raw stream has been detached"); \
"underlying buffer" rather than "raw stream"?

http://codereview.appspot.com/52075/diff/1/6#newcode1092
Line 1092: "raw stream has been detached"); \
same as above

http://codereview.appspot.com/52075/diff/1/6#newcode1112
Line 1112: Py_CLEAR(self->buffer);
Why not the simpler:
    buffer = self->buffer;
    self->buffer = NULL;
?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b