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.

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

Jump to bottom

Remove misleading sentence #11216

Merged
merged 2 commits into from
Apr 12, 2019
Merged

Remove misleading sentence #11216

merged 2 commits into from
Apr 12, 2019

Conversation

Hawk777
Copy link
Contributor

@Hawk777 Hawk777 commented Dec 18, 2018

The sentence, “It inherits BufferedReader and BufferedWriter, and
further supports seek() and tell() functionality,” is misleading: it
suggests that BufferedRandom is special in that it is seekable, whereas
BufferedReader and BufferedWriter are not. However, this is not the
case: assuming the underlying raw stream is seekable (which is equally a
requirement of BufferedRandom), then BufferedReader and BufferedWriter
are seekable. Remove the misleading sentence.

@Hawk777
Remove misleading sentence
2b99f16 
The sentence, “It inherits BufferedReader and BufferedWriter, and
further supports seek() and tell() functionality,” is misleading: it
suggests that BufferedRandom is special in that it is seekable, whereas
BufferedReader and BufferedWriter are not. However, this is not the
case: assuming the underlying raw stream is seekable (which is equally a
requirement of BufferedRandom), then BufferedReader and BufferedWriter
*are* seekable. Remove the misleading sentence.
@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Dec 18, 2018
@csabella csabella requested a review from benjaminp April 11, 2019 15:39
benjaminp
benjaminp requested changes Apr 11, 2019
View reviewed changes
Copy link
Contributor

@benjaminp benjaminp left a comment

Choose a reason for hiding this comment

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

I think we still need to say something about seek & tell otherwise it's not clear what BufferedRandom is for at all unless you know what a "random" stream is. We could say seek and tell must be supported.

@bedevere-bot
Copy link

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.

@Hawk777
Indicate that seek and tell are mandatory
b2c0035 
For BufferedReader and BufferedWriter, seek and tell operations are
optional (they may or may not exist based on the underlying stream). For
BufferedRandom, they are mandatory: a BufferedRandom should not be
constructed over an unseekable underlying stream. Document this.
@Hawk777
Copy link
Contributor Author

Hawk777 commented Apr 12, 2019

How about this? I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@benjaminp: please review the changes made to this pull request.

@benjaminp benjaminp merged commit b13552c into python:master Apr 12, 2019
@miss-islington
Copy link
Contributor

Thanks @Hawk777 for the PR, and @benjaminp for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.7.
🐍🍒⛏🤖

@benjaminp
Copy link
Contributor

Thank you for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjaminp benjaminp benjaminp requested changes

Assignees
No one assigned
Labels
docs Documentation in the Doc dir skip issue skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Hawk777 @bedevere-bot @miss-islington @benjaminp @the-knights-who-say-ni