-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Clarify last element in str.{r,}splitn documentation #36930
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
Conversation
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
src/libcollections/str.rs
Outdated
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, as a whole, your changes are an improvement and make it easier to understand this concept.
If a
countth substring
I personally stumbled on this though. It took me a few re-reads to understand this is talking about "substring with countth number of items". I can't think of a better suggestion at the moment without making it too verbose. Going to think about this...
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.
For non-english reader, this is particurlarly hard.
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.
How about this:
If
countsubstrings are returned, the last substring i.e. thecountth substring will contain the remainder of the string.
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's better. Or maybe even:
If
countsubstrings are returned, the last substring (thecountth substring) will contain the remainder of the string.
What do you think @GuillaumeGomez ?
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.
Crazy idea: rename the parameter from count to n (slice::splitn's parameter is called n). Then we can say "the nth substring" which is probably easier to parse.
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.
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'm generally against single letter variable names, but considering the method name is splitn, I think n makes sense. What do you think @angelsl? If you're alright with it, can you rename the parameter and update the instances of count in the doc comment?
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.
Sounds good to me. I've amended the commit.
|
Looks great, thanks! r=me |
|
@bors: r=frewsxcv rollup |
|
📌 Commit a4e9c39 has been approved by |
Clarify last element in str.{r,}splitn documentation
An attempt at rust-lang#36202.
I'm not sure if my wording is actually clearer, to be honest...
An attempt at #36202.
I'm not sure if my wording is actually clearer, to be honest...