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

#21859: Add Python implementation of FileIO

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 3 months ago by storchaka+cpython
Modified:
4 years, 5 months ago
Reviewers:
victor.stinner, 4kir4.1i, shadowranger+python, berker.peksag
CC:
AntoinePitrou, haypo, Benjamin Peterson, stutzbach, alex, devnull_psf.upfronthosting.co.za, Martin Panter, piotr.dobrogost, storchaka
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 34

Patch Set 3 #

Total comments: 26

Patch Set 4 #

Total comments: 17

Patch Set 5 #

Total comments: 4

Patch Set 6 #

Total comments: 39

Patch Set 7 #

Patch Set 8 #

Total comments: 26

Patch Set 9 #

Total comments: 32

Patch Set 10 #

Patch Set 11 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/_pyio.py View 1 2 3 4 5 6 7 8 9 10 2 chunks +15 lines, -13 lines 0 comments Download
Lib/test/support/__init__.py View 1 2 3 4 5 6 7 8 9 10 2 chunks +8 lines, -1 line 0 comments Download
Lib/test/test_fileio.py View 1 2 3 4 5 6 7 8 9 10 3 chunks +26 lines, -25 lines 0 comments Download
Lib/test/test_file.py View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -2 lines 0 comments Download
Lib/test/test_io.py View 1 2 3 4 5 6 7 8 9 10 5 chunks +33 lines, -30 lines 0 comments Download
Lib/test/test_largefile.py View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -2 lines 0 comments Download

Messages

Total messages: 20
victor.stinner_gmail.com
Great work Serhiy! This is a first and quick review. I didn't compare the C ...
5 years, 2 months ago #1
storchaka_gmail.com
http://bugs.python.org/review/21859/diff/12337/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/12337/Lib/_pyio.py#newcode1391 Lib/_pyio.py:1391: __fd = -1 On 2014/07/05 15:10:03, haypo wrote: > ...
5 years, 2 months ago #2
victor.stinner_gmail.com
The new patch looks much better, thanks! Here is a second review. I still did ...
5 years, 2 months ago #3
storchaka_gmail.com
Thank for your review Victor. http://bugs.python.org/review/21859/diff/12345/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/12345/Lib/_pyio.py#newcode1406 Lib/_pyio.py:1406: writing so this mode ...
5 years, 2 months ago #4
victor.stinner_gmail.com
http://bugs.python.org/review/21859/diff/12345/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/12345/Lib/_pyio.py#newcode1480 Lib/_pyio.py:1480: raise On 2014/07/22 13:28:59, storchaka wrote: > I thought ...
5 years, 2 months ago #5
storchaka_gmail.com
http://bugs.python.org/review/21859/diff/12448/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/12448/Lib/_pyio.py#newcode1501 Lib/_pyio.py:1501: except AttributeError: On 2014/07/22 13:46:01, haypo wrote: > I ...
5 years, 2 months ago #6
victor.stinner_gmail.com
http://bugs.python.org/review/21859/diff/12448/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/12448/Lib/_pyio.py#newcode1511 Lib/_pyio.py:1511: import msvcrt You may move the import at the ...
5 years, 2 months ago #7
akira
http://bugs.python.org/review/21859/diff/12448/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/12448/Lib/_pyio.py#newcode1534 Lib/_pyio.py:1534: stacklevel=2) fileio.c prints `self` here, not `self.name` fileio.c generates ...
5 years, 1 month ago #8
storchaka_gmail.com
http://bugs.python.org/review/21859/diff/12448/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/12448/Lib/_pyio.py#newcode1511 Lib/_pyio.py:1511: import msvcrt On 2014/07/22 14:30:45, haypo wrote: > You ...
5 years, 1 month ago #9
Josh.R
Little weirded out by the use of class variables that exist only to be instance ...
5 years, 1 month ago #10
storchaka_gmail.com
http://bugs.python.org/review/21859/diff/12559/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/12559/Lib/_pyio.py#newcode1398 Lib/_pyio.py:1398: _closefd = True On 2014/08/01 01:59:16, josh.rosenberg wrote: > ...
5 years, 1 month ago #11
victor.stinner_gmail.com
First quick review, I didn't have time to compare exactly the Python with the C ...
4 years, 6 months ago #12
storchaka_gmail.com
http://bugs.python.org/review/21859/diff/12974/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/12974/Lib/_pyio.py#newcode1474 Lib/_pyio.py:1474: try: On 2015/03/23 11:31:24, haypo wrote: > Please add ...
4 years, 6 months ago #13
berkerpeksag
http://bugs.python.org/review/21859/diff/14319/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/14319/Lib/_pyio.py#newcode1394 Lib/_pyio.py:1394: """Open a file. The mode can be 'r', 'w', ...
4 years, 6 months ago #14
storchaka_gmail.com
For most questions there is one answer -- this is the same as in C ...
4 years, 6 months ago #15
berkerpeksag
http://bugs.python.org/review/21859/diff/14319/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/14319/Lib/_pyio.py#newcode1424 Lib/_pyio.py:1424: if not set(mode) <= set('xrwab+'): On 2015/03/23 15:50:33, storchaka ...
4 years, 6 months ago #16
victor.stinner_gmail.com
http://bugs.python.org/review/21859/diff/14319/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/14319/Lib/_pyio.py#newcode1397 Lib/_pyio.py:1397: when opened for writing. A `FileExistsError` will be raised ...
4 years, 6 months ago #17
victor.stinner_gmail.com
http://bugs.python.org/review/21859/diff/12974/Lib/_pyio.py File Lib/_pyio.py (right): http://bugs.python.org/review/21859/diff/12974/Lib/_pyio.py#newcode1493 Lib/_pyio.py:1493: if not noinherit_flag: On 2015/03/23 13:30:22, storchaka wrote: > ...
4 years, 6 months ago #18
victor.stinner_gmail.com
https://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py File Lib/_pyio.py (right): https://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1418 Lib/_pyio.py:1418: elif isinstance(file, int): You may just write "if" here. ...
4 years, 5 months ago #19
storchaka_gmail.com
4 years, 5 months ago #20
http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py
File Lib/_pyio.py (right):

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1418
Lib/_pyio.py:1418: elif isinstance(file, int):
On 2015/04/10 09:50:23, haypo wrote:
> You may just write "if" here.

Done.

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1421
Lib/_pyio.py:1421: raise ValueError('Negative filedescriptor')
On 2015/04/10 09:50:23, haypo wrote:
> typo: need a space.

Done.

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1460
Lib/_pyio.py:1460: flags |= binary_flag
On 2015/04/10 09:50:23, haypo wrote:
> You can avoid binary_flag variable here:
> 
> flags |= getattr(os, 'O_BINARY', 0)

Done. Originally this flag was used multiple times.

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1496
Lib/_pyio.py:1496: self._blksize = blksize
On 2015/04/10 09:50:23, haypo wrote:
> May you should put self._blksize = DEFAULT_BUFFER_SIZE in an else block
instead
> of setting it before, it would be more readable.

Done.

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1504
Lib/_pyio.py:1504: # For consistent behaviour, we explicitly seek to the
On 2015/04/10 09:50:23, haypo wrote:
> Hum, it's more to have a *portable* behaviour. Some platforms seek at open,
some
> others don't. (If the comment it modified, it should also be modified in the C
> code.)

Isn't "consistent" a synonym of "portable" here?

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1518
Lib/_pyio.py:1518: stacklevel=2)
On 2015/04/10 09:50:23, haypo wrote:
> Oh, this destructor remembers me the issue #19829: "pyio.BufferedReader and
> _pyio.TextIOWrapper destructor don't emit ResourceWarning if the file is not
> closed". You may want to take a look.
> 
> I wrote a hack to show the backtrace when a ResourceWarning is emitted:
> https://bitbucket.org/haypo/misc/src/tip/python/res_warn.py#cl-62
> 
> (Maybe it's easier to hack warnings.showwarning?)

I'm aware about #19829. Emitting a ResourceWarning in io classes is implemented
with a hack. I want to avoid this hack for now in Python implementation. Latter
I'll try to implement this in more direct manner.

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1586
Lib/_pyio.py:1586: b = os.read(self._fd, n)
On 2015/04/10 09:50:23, haypo wrote:
> I don't like variable name with a single letter. You can for example rename b
to
> chunk.

Done. But this variable is used in similar code across all the stdlib.

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1609
Lib/_pyio.py:1609: The number of bytes actually written is returned.
On 2015/04/10 09:50:23, haypo wrote:
> Hum, the docstring doesn't mention None returned for non-blocking files.
> Suggestion:
> 
> In non-blocking mode, returns None if the write would block.
> 
> Again, the C code must be updated too.

Done.

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1623
Lib/_pyio.py:1623: (move relative to current position, positive or negative),
and 2 (move
On 2015/04/10 09:50:23, haypo wrote:
> Hum, it sounds wrong to constant hardcoded constants. io.SEEK_SET,
io.SEEK_CUR,
> io.SEEK_END should be used instead.

Done. The SEEK_* constants are new in 3.1. Most code in the stdlib uses
hardcoded 0, 1, 2.

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1634
Lib/_pyio.py:1634: """tell() -> int.  Current file position"""
On 2015/04/10 09:50:23, haypo wrote:
> It may be useful to mention that even tell() can raise OSError for non
seekable
> files (according to the implementation of seekable().)

Done.

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1655
Lib/_pyio.py:1655: called more than once without error.  Changes the fileno to
-1.
On 2015/04/10 09:50:23, haypo wrote:
> Hum, I don't see how the fileno is set to -1.

Done.

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1687
Lib/_pyio.py:1687: """"File descriptor".
On 2015/04/10 09:50:23, haypo wrote:
> The C comment contains "fileno() -> int." This information is useful and
should
> be added here.

Done.

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1695
Lib/_pyio.py:1695: """True if the file is connected to a tty device."""
On 2015/04/10 09:50:23, haypo wrote:
> Hum, tty should be written TTY. (same issue in C docstring)

Done.

http://bugs.python.org/review/21859/diff/14485/Lib/_pyio.py#newcode1701
Lib/_pyio.py:1701: """True if the file descriptor will be closed"""
On 2015/04/10 09:50:23, haypo wrote:
> Hum, a final dot is missing. It may help to mention "will be closed *by
> close()*".

Done.

http://bugs.python.org/review/21859/diff/14485/Lib/test/test_file_eintr.py
File Lib/test/test_file_eintr.py (right):

http://bugs.python.org/review/21859/diff/14485/Lib/test/test_file_eintr.py#ne...
Lib/test/test_file_eintr.py:42: return ('import %s as io ;'
On 2015/04/10 09:50:24, haypo wrote:
> Hum, maybe you can use {modname} with format() to write {modname}.FileIO
below,
> instead of "as io".

The code with the io alias looks more clean to me than the code with multiple
{modname}.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+