Skip to content

implement rfc 1054: split_whitespace() fn, deprecate words() #24563

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

Merged
merged 2 commits into from
Apr 22, 2015

Conversation

kwantam
Copy link
Contributor

@kwantam kwantam commented Apr 18, 2015

For now, words() is left in (but deprecated), and Words is a type alias for
struct SplitWhitespace.

Also cleaned up references to str.words() throughout codebase.

Closes #15628

@rust-highfive
Copy link
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@kwantam
Copy link
Contributor Author

kwantam commented Apr 18, 2015

Testing this on my end, but comments appreciated, especially whether split_whitespace() should immediately be stabilized, whether words() should stay as deprecated for now, etc.

Local tests finished successfully.

@alexcrichton
Copy link
Member

Looks good to me! My only concern would be the insta-stabilization of the function, but this is just a renaming of the function so I would be fine stabilizing it (we've done this many times before).

Basically r+ from me, but would like to r? @aturon as well.

@rust-highfive rust-highfive assigned aturon and unassigned alexcrichton Apr 18, 2015
@alexcrichton
Copy link
Member

r? @aturon

@@ -1739,27 +1739,50 @@ impl str {
UnicodeStr::grapheme_indices(&self[..], is_extended)
}

/// An iterator over the non-empty words of `self`.
/// An iterator over non-whitespace substrings of `self`,
Copy link
Member

Choose a reason for hiding this comment

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

What’re non-whitespace substrings?

@kwantam
Copy link
Contributor Author

kwantam commented Apr 20, 2015

@nagisa updated documentation to address your comments.

Also rebased this branch for good measure.

@nagisa
Copy link
Member

nagisa commented Apr 20, 2015

@kwantam thanks!

LGTM.

@aturon
Copy link
Member

aturon commented Apr 21, 2015

Can you mark the #[stable] entries with something like:

#[stable(feature = "split_whitespace", since = "1.1.0")]

since this will be shipping in 1.1, not 1.0?

Otherwise, r=me!

@kwantam
Copy link
Contributor Author

kwantam commented Apr 21, 2015

Modified per @aturon's comment.

/// let some_words = " Mary had\ta little \n\t lamb";
/// let v: Vec<&str> = some_words.words().collect();
///
/// assert_eq!(v, ["Mary", "had", "a", "little", "lamb"]);
/// ```
#[deprecated(reason = "words() will be removed. Use split_whitespace() instead",
since = "1.0.0")]
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be 1.1.0 as well?

@kwantam
Copy link
Contributor Author

kwantam commented Apr 21, 2015

@alexcrichton good catch.

@alexcrichton
Copy link
Member

@bors: r+ a085352

@alexcrichton alexcrichton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 21, 2015
@alexcrichton
Copy link
Member

Nominating for the beta milestone to make sure we cherry-pick the destabilization of str::Words, but we should not cherry-pick this entire PR.

@kwantam
Copy link
Contributor Author

kwantam commented Apr 21, 2015

@alexcrichton do you want me to split this into two separate commits to make that easier?

@alexcrichton
Copy link
Member

@kwantam sure!

kwantam added 2 commits April 21, 2015 15:31
Words struct was stabilied by mistake. Unstabilize.
For now, words() is left in (but deprecated), and Words is a type alias for
struct SplitWhitespace.

Also cleaned up references to s.words() throughout codebase.

Closes rust-lang#15628
@kwantam
Copy link
Contributor Author

kwantam commented Apr 21, 2015

Let's try that again.

f43c86c should applies cleanly against 1.0.0-beta tag.

@alexcrichton
Copy link
Member

@bors: r+ c361e13

@alexcrichton
Copy link
Member

@bors: p=1

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 21, 2015
For now, words() is left in (but deprecated), and Words is a type alias for
struct SplitWhitespace.

Also cleaned up references to str.words() throughout codebase.

Closes rust-lang#15628
@bors bors merged commit c361e13 into rust-lang:master Apr 22, 2015
@kwantam kwantam deleted the rfc_1054 branch April 22, 2015 19:43
@pnkfelix
Copy link
Member

going from nominated to (nominated, accepted), again on just cherry-picking the destabilization of Words struct.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 23, 2015
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 23, 2015
@kwantam kwantam restored the rfc_1054 branch April 23, 2015 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

&str.words should be using the unicode word boundary algorithm
7 participants