Skip to content
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.

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

Fix bpo-30526: Add TextIOWrapper.reconfigure() #1922

Merged
merged 4 commits into from Jun 3, 2017

Conversation

Projects
None yet
4 participants
@pitrou
Copy link
Member

commented Jun 2, 2017

No description provided.

@mention-bot

This comment has been minimized.

Copy link

commented Jun 2, 2017

@pitrou, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @serhiy-storchaka and @birkenfeld to be potential reviewers.

@pitrou

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2017

@ncoghlan
Copy link
Contributor

left a comment

The actual change mostly looks good to me (just one comment on the specifics of the docs wording).

The commit message should make sure to explain that this doesn't just cover adding reconfigure, it also adds a read-only property to read back the write_through status that was set in either the constructor or the last call to reconfigure().


Reconfigure this text stream using new values for *line_buffering*
and *write_through*. Any ``None`` value will keep the current
value.

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Jun 3, 2017

Contributor

Rather than using value 3 times, this may be clearer with a mix of setting and argument:

Reconfigure this text stream using new settings for line_buffering and write_through. Passing None as an argument will retain the current setting for that parameter.

That's just a suggestion though, I don't think it's a blocker for merging the patch.

This comment has been minimized.

Copy link
@pitrou

pitrou Jun 3, 2017

Author Member

That's a good suggestion Nick, thank you.

@pitrou pitrou merged commit 3c2817b into python:master Jun 3, 2017

2 checks passed

bedevere/issue-number Issue number 30526 found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pitrou pitrou deleted the pitrou:reconfigure_textiowrapper branch Jun 3, 2017

mlouielu added a commit to mlouielu/cpython that referenced this pull request Jun 15, 2017

Fix bpo-30526: Add TextIOWrapper.reconfigure() and a TextIOWrapper.wr…
…ite_through attribute (python#1922)

* Fix bpo-30526: Add TextIOWrapper.reconfigure()

* Apply Nick's improved wording

* Update Misc/NEWS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.