Skip to content

bpo-21861: Improve _io.FileIO.__repr__ #14774

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

bpo-21861: Improve _io.FileIO.__repr__ #14774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions Lib/test/test_fileio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,27 @@ def test_none_args(self):
def test_reject(self):
self.assertRaises(TypeError, self.f.write, "Hello!")

def test_subclass_repr(self):
class CustomFileIO(self.FileIO):
pass

custom_file_io = CustomFileIO(TESTFN, 'w')

self.assertIn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is overly specific: it is testing the implementation's details (the specific formatting, including the order of the values) rather than the intent (that certain values are included).

I suggest something like:

r = repr(custom_file_io)
self.assertIn(clsname, r)
self.assertIn(custom_file_io.name, r)
self.assertIn(custom_file_io.mode, r)
self.assertIn("closefd=True", r)

This is also true for the similar assertion below, and for those in the TextIOWrapper test as well.

"CustomFileIO name=%r mode=%r closefd=True>" %
(custom_file_io.name, custom_file_io.mode),
repr(custom_file_io))

del custom_file_io.name
self.assertIn(
"CustomFileIO fd=%r mode=%r closefd=True>" %
(custom_file_io.fileno(), custom_file_io.mode),
repr(custom_file_io))

custom_file_io.close()
self.assertIn("CustomFileIO [closed]", repr(custom_file_io))


def testRepr(self):
self.assertEqual(repr(self.f),
"<%s.FileIO name=%r mode=%r closefd=True>" %
Expand Down
24 changes: 24 additions & 0 deletions Lib/test/test_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2630,6 +2630,30 @@ def test_repr(self):
t.buffer.detach()
repr(t) # Should not raise an exception

def test_subclass_repr(self):
class SubTextIOWrapper(self.TextIOWrapper):
pass

clsname = 'SubTextIOWrapper'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be simpler and clearer to just hard-code the class name in the strings below.

raw = self.BytesIO("hello".encode("utf-8"))
b = self.BufferedReader(raw)
t = SubTextIOWrapper(b, encoding="utf-8")
self.assertRegex(repr(t),
r"%s encoding='utf-8'>" % clsname)
raw.name = "dummy"
self.assertRegex(repr(t),
r"%s name='dummy' encoding='utf-8'>" % clsname)
t.mode = "r"
self.assertRegex(repr(t),
r"%s name='dummy' mode='r' encoding='utf-8'>" %
clsname)
raw.name = b"dummy"
self.assertRegex(
repr(t),
r"%s name=b'dummy' mode='r' encoding='utf-8'>" % clsname)
t.buffer.detach()
repr(t) # Should not raise an exception

def test_recursive_repr(self):
# Issue #25455
raw = self.BytesIO()
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2019-07-14-13-12-40.bpo-21861.wxAr8e.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Remove the hardcoded classes names from ``_io`` module classes' ``__repr__`` in
Copy link
Contributor

@taleinat taleinat Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that simply mentioning FileIO and TextIOWrapper would be clearer than "_io module classes'.

Also I think phrasing this positively would be better, but this is a matter of style:

Use the object's actual class name in :meth:`_io.FileIO.__repr__` and
:meth:`_io.TextIOWrapper.__repr__`, to make these methods subclass friendly.

(For more info about the inline markup used above, see the devguide.)

favor of the actual object type name to make it more subclass friendly.
10 changes: 7 additions & 3 deletions Modules/_io/fileio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1080,22 +1080,26 @@ fileio_repr(fileio *self)
PyObject *nameobj, *res;

if (self->fd < 0)
return PyUnicode_FromFormat("<_io.FileIO [closed]>");
return PyUnicode_FromFormat(
"<%s [closed]>",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be good to have a test for different cases? Something like below in test_fileio.py to ensure C and Python implementations behave the same.

def test_custom_repr(self):
    class CustomFileIO(self.FileIO):
        pass

    custom_file_io = CustomFileIO(TESTFN, 'w')
    custom_file_io.close()
    self.assertIn("CustomFileIO", repr(custom_file_io))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR definitely needs tests such as the one proposed here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taleinat @tirkarthi thanks for comments. I added some tests as suggested.

Py_TYPE((PyObject *) self)->tp_name);

if (_PyObject_LookupAttrId((PyObject *) self, &PyId_name, &nameobj) < 0) {
return NULL;
}
if (nameobj == NULL) {
res = PyUnicode_FromFormat(
"<_io.FileIO fd=%d mode='%s' closefd=%s>",
"<%s fd=%d mode='%s' closefd=%s>",
Py_TYPE((PyObject *) self)->tp_name,
self->fd, mode_string(self), self->closefd ? "True" : "False");
}
else {
int status = Py_ReprEnter((PyObject *)self);
res = NULL;
if (status == 0) {
res = PyUnicode_FromFormat(
"<_io.FileIO name=%R mode='%s' closefd=%s>",
"<%s name=%R mode='%s' closefd=%s>",
Py_TYPE((PyObject *) self)->tp_name,
nameobj, mode_string(self), self->closefd ? "True" : "False");
Py_ReprLeave((PyObject *)self);
}
Expand Down
2 changes: 1 addition & 1 deletion Modules/_io/textio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2886,7 +2886,7 @@ textiowrapper_repr(textio *self)

CHECK_INITIALIZED(self);

res = PyUnicode_FromString("<_io.TextIOWrapper");
res = PyUnicode_FromFormat("<%s", Py_TYPE((PyObject *) self)->tp_name);
if (res == NULL)
return NULL;

Expand Down
12 changes: 9 additions & 3 deletions Modules/_io/winconsoleio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1034,13 +1034,19 @@ static PyObject *
winconsoleio_repr(winconsoleio *self)
{
if (self->handle == INVALID_HANDLE_VALUE)
return PyUnicode_FromFormat("<_io._WindowsConsoleIO [closed]>");
return PyUnicode_FromFormat(
"<%s [closed]>",
Py_TYPE((PyObject *) self)->tp_name);

if (self->readable)
return PyUnicode_FromFormat("<_io._WindowsConsoleIO mode='rb' closefd=%s>",
return PyUnicode_FromFormat(
"<%s mode='rb' closefd=%s>",
Py_TYPE((PyObject *) self)->tp_name,
self->closehandle ? "True" : "False");
if (self->writable)
return PyUnicode_FromFormat("<_io._WindowsConsoleIO mode='wb' closefd=%s>",
return PyUnicode_FromFormat(
"<%s mode='wb' closefd=%s>",
Py_TYPE((PyObject *) self)->tp_name,
self->closehandle ? "True" : "False");

PyErr_SetString(PyExc_SystemError, "_WindowsConsoleIO has invalid mode");
Expand Down