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

doc/core: fix description of nth function #27612

Merged
merged 1 commit into from
Aug 11, 2015

Conversation

vincentbernat
Copy link
Contributor

The "nth" element can be confusing. In an array context, we know indexes
start from 0 but one may believe this is not the case with "nth". For
example, would .nth(1) return the first (1th/1st) or the second
element? Rephrase a bit to be less confusing.

r? @steveklabnik

The "nth" element can be confusing. In an array context, we know indexes
start from 0 but one may believe this is not the case with "nth". For
example, would `.nth(1)` return the first (1th/1st) or the second
element?  Rephrase a bit to be less confusing.
@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 @steveklabnik (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. 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.

@vincentbernat
Copy link
Contributor Author

BTW, dunno if the source of the bot is available somewhere, but its language is confusing as well:

The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed.

I think something is missing.

@@ -148,7 +148,7 @@ pub trait Iterator {
last
}

/// Loops through `n` iterations, returning the `n`th element of the
/// Loops through `n` iterations, returning the next (`n+1`th) element of the
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't use a formulation with “n + 1 th”: the meaning of “nth” as used in this sentence is using a different indexing convention than the nth in the method name. Best to word it differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about:

Loops through n iterations, returning the next (nth) element of the iterator. The first element returned by the iterator is the "zeroth" one.

Or otherwise, just say it completely differently:

Returns the element at index n by looping through n iterations and returning the next element of the iterator.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer the different version. Perhaps say skip and next element after that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So:

Returns the element at index n by skipping the n first elements and returning the next one.

Or did you mean:

Skip the n first elements of the iterator and return the next one.

Copy link
Member

Choose a reason for hiding this comment

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

either you like is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commit updated with the new version.

@steveklabnik
Copy link
Member

@vincentbernat

BTW, dunno if the source of the bot is available somewhere,

https://github.com/nrc/highfive

(it is a confusing sentence)

@vincentbernat
Copy link
Contributor Author

@steveklabnik this is already fixed: rust-lang/highfive@d4ddb75

@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Aug 9, 2015

📌 Commit 721dc47 has been approved by steveklabnik

@bors
Copy link
Contributor

bors commented Aug 9, 2015

⌛ Testing commit 721dc47 with merge fd1e7d8...

@bors
Copy link
Contributor

bors commented Aug 9, 2015

💔 Test failed - auto-linux-32-opt

@dotdash
Copy link
Contributor

dotdash commented Aug 10, 2015

@bors retry

Manishearth added a commit to Manishearth/rust that referenced this pull request Aug 11, 2015
…teveklabnik

The "nth" element can be confusing. In an array context, we know indexes
start from 0 but one may believe this is not the case with "nth". For
example, would `.nth(1)` return the first (1th/1st) or the second
element?  Rephrase a bit to be less confusing.

r? @steveklabnik
bors added a commit that referenced this pull request Aug 11, 2015
@bors bors merged commit 721dc47 into rust-lang:master Aug 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants