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

beezz · 2019-07-14T10:47:31Z

Instead of hard-coding the class name in the __repr__ string, resolve the actual one which makes the __repr__ more subclass friendly.

https://bugs.python.org/issue21861

the-knights-who-say-ni · 2019-07-14T10:47:34Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

serhiy-storchaka · 2019-07-14T17:11:10Z

What about other io classes and the Python implementation (_pyio)?

beezz · 2019-07-15T05:38:47Z

@serhiy-storchaka Thanks for your comment. I added remaining classes from the _io module and checked the _pyio implementation which already uses correct class names.

tirkarthi · 2019-07-15T09:13:58Z

Modules/_io/fileio.c

@@ -1074,22 +1074,26 @@ fileio_repr(fileio *self)
    PyObject *nameobj, *res;

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


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))

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

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

taleinat · 2019-07-17T19:47:42Z

@beezz, thanks for this PR!

Please link your user on bugs.python.org to your GitHub user so that our CLA bot can recognize that you've signed our CLA.

csabella · 2020-02-06T02:25:18Z

@beezz, please address the review comments. Thank you!

beezz · 2020-02-13T16:03:50Z

@beezz, please address the review comments. Thank you!

@csabella Thanks for the comment. I tried to address the requested changes and added the unit tests.

Instead of hardcoding the class name in the __repr__ string, resolve the actuall one which makes the __repr__ more subclass friendly.

taleinat

Looking good! I think this is almost ready to go.

I have just one significant request regarding the tests (see inline comment).

Also a couple of minor style suggestions and some nitpicking about the NEWS entry, but those aren't critical.

taleinat · 2020-02-21T13:38:07Z

Lib/test/test_io.py

+        class SubTextIOWrapper(self.TextIOWrapper):
+            pass
+
+        clsname = 'SubTextIOWrapper'


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

taleinat · 2020-02-21T13:51:10Z

Lib/test/test_fileio.py

+
+        custom_file_io = CustomFileIO(TESTFN, 'w')
+
+        self.assertIn(


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.

taleinat · 2020-02-21T13:58:36Z

Misc/NEWS.d/next/Core and Builtins/2019-07-14-13-12-40.bpo-21861.wxAr8e.rst

@@ -0,0 +1,2 @@
+Remove the hardcoded classes names from ``_io`` module classes' ``__repr__`` in


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.)

bedevere-bot · 2020-02-21T14:00:20Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

taleinat · 2020-02-21T14:03:02Z

Also, @beezz, please avoid rebasing and force-pushing branches which already have an open PR. It is much better, for various reasons, to simply merge the base branch into the PR branch and push the merged branch (this doesn't require force pushing, which is a good sign!).

We always squash PR branches before merging them, so the end result is the same either way.

For more details on the recommended PR workflow, see this section of devguide.

erlend-aasland · 2024-01-09T17:49:15Z

Superseded by #30824

the-knights-who-say-ni added the CLA not signed label Jul 14, 2019

bedevere-bot added the awaiting review label Jul 14, 2019

beezz force-pushed the bpo-21861-_io.FileIO-repr-subclass-friendly branch from dc62f03 to 0a02fcb Compare July 14, 2019 11:13

beezz force-pushed the bpo-21861-_io.FileIO-repr-subclass-friendly branch 2 times, most recently from 8109726 to 47ed9a3 Compare July 15, 2019 05:36

tirkarthi reviewed Jul 15, 2019

View reviewed changes

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Aug 28, 2019

csabella added awaiting changes and removed awaiting review labels Feb 6, 2020

beezz force-pushed the bpo-21861-_io.FileIO-repr-subclass-friendly branch from 47ed9a3 to bd9440e Compare February 13, 2020 15:58

beezz force-pushed the bpo-21861-_io.FileIO-repr-subclass-friendly branch from bd9440e to 3b80300 Compare February 13, 2020 16:15

bpo-21861: Improve _io classes __repr__

996fb04

Instead of hardcoding the class name in the __repr__ string, resolve the actuall one which makes the __repr__ more subclass friendly.

beezz force-pushed the bpo-21861-_io.FileIO-repr-subclass-friendly branch from 3b80300 to 996fb04 Compare February 13, 2020 16:29

csabella added awaiting review and removed awaiting changes labels Feb 14, 2020

csabella requested review from taleinat and tirkarthi February 14, 2020 09:57

taleinat requested changes Feb 21, 2020

View reviewed changes

bedevere-bot removed the awaiting review label Feb 21, 2020

bedevere-bot added the awaiting changes label Feb 21, 2020

ezio-melotti removed the CLA signed label Jul 13, 2022

erlend-aasland closed this Jan 9, 2024

serhiy-storchaka mentioned this pull request Apr 10, 2022

io class name are hardcoded in reprs #66060

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

beezz commented Jul 14, 2019 •

edited by taleinat

the-knights-who-say-ni commented Jul 14, 2019

serhiy-storchaka commented Jul 14, 2019

beezz commented Jul 15, 2019

tirkarthi Jul 15, 2019

taleinat Jul 17, 2019

beezz Feb 13, 2020

taleinat commented Jul 17, 2019 •

edited

csabella commented Feb 6, 2020

beezz commented Feb 13, 2020

taleinat left a comment

taleinat Feb 21, 2020

taleinat Feb 21, 2020

taleinat Feb 21, 2020 •

edited

bedevere-bot commented Feb 21, 2020

taleinat commented Feb 21, 2020 •

edited

erlend-aasland commented Jan 9, 2024

		@@ -0,0 +1,2 @@
		Remove the hardcoded classes names from ``_io`` module classes' ``__repr__`` in

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

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

Conversation

beezz commented Jul 14, 2019 • edited by taleinat

the-knights-who-say-ni commented Jul 14, 2019

serhiy-storchaka commented Jul 14, 2019

beezz commented Jul 15, 2019

tirkarthi Jul 15, 2019

Choose a reason for hiding this comment

taleinat Jul 17, 2019

Choose a reason for hiding this comment

beezz Feb 13, 2020

Choose a reason for hiding this comment

taleinat commented Jul 17, 2019 • edited

csabella commented Feb 6, 2020

beezz commented Feb 13, 2020

taleinat left a comment

Choose a reason for hiding this comment

taleinat Feb 21, 2020

Choose a reason for hiding this comment

taleinat Feb 21, 2020

Choose a reason for hiding this comment

taleinat Feb 21, 2020 • edited

Choose a reason for hiding this comment

bedevere-bot commented Feb 21, 2020

taleinat commented Feb 21, 2020 • edited

erlend-aasland commented Jan 9, 2024

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

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

beezz commented Jul 14, 2019 •

edited by taleinat

taleinat commented Jul 17, 2019 •

edited

taleinat Feb 21, 2020 •

edited

taleinat commented Feb 21, 2020 •

edited