GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-21861: Improve _io.FileIO.__repr__ by beezz · Pull Request #14774 · python/cpython

Conversation

@beezz

@beezz beezz commented Jul 14, 2019 •
edited by taleinat
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Jul 14, 2019
@the-knights-who-say-ni

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

Copy link
Copy Markdown

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting review label Jul 14, 2019
@beezz
beezz force-pushed the bpo-21861-_io.FileIO-repr-subclass-friendly branch from dc62f03 to 0a02fcb Compare July 14, 2019 11:13
@serhiy-storchaka

serhiy-storchaka commented Jul 14, 2019

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@beezz
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
@beezz

beezz commented Jul 15, 2019

Copy link
Copy Markdown
Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tirkarthi
tirkarthi reviewed Jul 15, 2019
View reviewed changes
Comment thread Modules/_io/fileio.c
if (self->fd < 0)
return PyUnicode_FromFormat("<_io.FileIO [closed]>");
return PyUnicode_FromFormat(
"<%s [closed]>",

@tirkarthi tirkarthi Jul 15, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@taleinat taleinat Jul 17, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@beezz beezz Feb 13, 2020

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@taleinat

taleinat commented Jul 17, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels Aug 28, 2019
@csabella

csabella commented Feb 6, 2020

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@csabella csabella added awaiting changes and removed awaiting review labels Feb 6, 2020
@beezz
beezz force-pushed the bpo-21861-_io.FileIO-repr-subclass-friendly branch from 47ed9a3 to bd9440e Compare February 13, 2020 15:58
@beezz

beezz commented Feb 13, 2020

Copy link
Copy Markdown
Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@beezz
beezz force-pushed the bpo-21861-_io.FileIO-repr-subclass-friendly branch from bd9440e to 3b80300 Compare February 13, 2020 16:15
Michal Kuffa
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
beezz force-pushed the bpo-21861-_io.FileIO-repr-subclass-friendly branch from 3b80300 to 996fb04 Compare February 13, 2020 16:29
@csabella csabella added awaiting review and removed awaiting changes labels Feb 14, 2020
@csabella
csabella requested review from taleinat and tirkarthi February 14, 2020 09:57
taleinat
taleinat suggested changes Feb 21, 2020
View reviewed changes

@taleinat taleinat left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_io.py
class SubTextIOWrapper(self.TextIOWrapper):
pass

clsname = 'SubTextIOWrapper'

@taleinat taleinat Feb 21, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_fileio.py

custom_file_io = CustomFileIO(TESTFN, 'w')

self.assertIn(

@taleinat taleinat Feb 21, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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

@taleinat taleinat Feb 21, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting review label Feb 21, 2020
@bedevere-bot

bedevere-bot commented Feb 21, 2020

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Feb 21, 2020
@taleinat

taleinat commented Feb 21, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@erlend-aasland erlend-aasland closed this Jan 9, 2024
@erlend-aasland

erlend-aasland commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

Superseded by #30824
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka serhiy-storchaka mentioned this pull request Apr 10, 2022
io class name are hardcoded in reprs #66060
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tirkarthi tirkarthi Awaiting requested review from tirkarthi
1 more reviewer
@taleinat taleinat taleinat requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@beezz @the-knights-who-say-ni @serhiy-storchaka @taleinat @csabella @bedevere-bot @erlend-aasland @tirkarthi @ezio-melotti
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.