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 a misleading statement in Iterator.nth() #39174

Merged
merged 3 commits into from
Feb 11, 2017

Conversation

rspeer
Copy link
Contributor

@rspeer rspeer commented Jan 19, 2017

The Iterator.nth() documentation says "Note that all preceding elements will be consumed". I assumed from that that the preceding elements would be the only ones that were consumed, but in fact the returned element is consumed as well.

The way I read the documentation, I assumed that nth(0) would not discard anything (there are 0 preceding elements, and maybe it just peeks at the start of the iterator somehow), so I added a sentence clarifying that it does. I also rephrased it to avoid the stunted "i.e." phrasing.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@bluss
Copy link
Member

bluss commented Jan 20, 2017

The nth element itself is not really "consumed" (without redeem) since it is returned. Can we have some explanation in between maybe, for example to clarify that the iterator's state is updated to be as if n + 1 calls to .next() had been called (but preferably with a simpler explanation than that).

Edit: Just noticed a wrinkle, it's as if at most n + 1 calls to next had been called (less if the end was reached).

@BurntSushi
Copy link
Member

@rspeer What are your thoughts on bluss' suggestion?

@BurntSushi BurntSushi added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 23, 2017
@rspeer
Copy link
Contributor Author

rspeer commented Jan 23, 2017

I was making a distinction in the docstring between "consumed" and "discarded"; it seems to me that the returned element is "consumed", in that it's not in the iterator anymore, but only the preceding elements are "discarded". But maybe this is specific Rust terminology that doesn't work like I think. I messed up the distinction I was trying to make when making the initial comment on this PR, incidentally.

bluss's clarification that it acts like at most n + 1 calls to .next() should be helpful, yes.

@bluss
Copy link
Member

bluss commented Jan 24, 2017

It does seem consistent, maybe the PR text can be expanded to say "will be consumed from the iterator".

@alexcrichton
Copy link
Member

I like the terminology "consumed from the iterator" for this as well, @rspeer want to update the PR?

Rob Speer and others added 3 commits February 10, 2017 01:31
The `Iterator.nth()` documentation says "Note that all preceding elements will be consumed". I assumed from that that the preceding elements would be the *only* ones that were consumed, but in fact the returned element is consumed as well.

The way I read the documentation, I assumed that `nth(0)` would not discard anything (as there are 0 preceding elements), so I added a sentence clarifying that it does. I also rephrased it to avoid the stunted "i.e." phrasing.
@rspeer
Copy link
Contributor Author

rspeer commented Feb 10, 2017

Alright, I clarified. I also moved the paragraph down (because it benefits from having already explained that n counts from 0).

@alexcrichton
Copy link
Member

@bors: r+ rollup

Thanks!

@bors
Copy link
Contributor

bors commented Feb 10, 2017

📌 Commit 11d36ae has been approved by alexcrichton

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 11, 2017
…hton

Fix a misleading statement in `Iterator.nth()`

The `Iterator.nth()` documentation says "Note that all preceding elements will be consumed". I assumed from that that the preceding elements would be the *only* ones that were consumed, but in fact the returned element is consumed as well.

The way I read the documentation, I assumed that `nth(0)` would not discard anything (there are 0 preceding elements, and maybe it just peeks at the start of the iterator somehow), so I added a sentence clarifying that it does. I also rephrased it to avoid the stunted "i.e." phrasing.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 11, 2017
…hton

Fix a misleading statement in `Iterator.nth()`

The `Iterator.nth()` documentation says "Note that all preceding elements will be consumed". I assumed from that that the preceding elements would be the *only* ones that were consumed, but in fact the returned element is consumed as well.

The way I read the documentation, I assumed that `nth(0)` would not discard anything (there are 0 preceding elements, and maybe it just peeks at the start of the iterator somehow), so I added a sentence clarifying that it does. I also rephrased it to avoid the stunted "i.e." phrasing.
bors added a commit that referenced this pull request Feb 11, 2017
Rollup of 9 pull requests

- Successful merges: #39174, #39660, #39676, #39692, #39701, #39710, #39721, #39724, #39725
- Failed merges:
@bors bors merged commit 11d36ae into rust-lang:master Feb 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants