-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Tracking Issue for str_split_once #74773
Comments
I wonder if it shouldn't return |
The way I am interpreting the current signature of Returning |
I've just used it in one of my projects and it felt very natural, I agree with your reasoning James |
Tried it today for a quick string matching script, and it felt really good to use. Not returning the separator felt consistent with edit: probably two things to consider: |
I have used it here. |
I seems like the current implementation compiles in a bunch of panic checks, which shouldn't be necessary: https://rust.godbolt.org/z/WTTn6K |
Yeah, the impl should be changed to use unchecked indexing (I believe there's no safe API we can use here), but it probably makes sense to do this after stabilization. |
I think we should stabilize the API as is:
I think we should not complicate the API to return the ranged matched by the delimiter pattern: this makes the common case more difficult to use, and it goes inline with other pattern-using APIs. We can always add I don't think we should change the Is anyone willing to submit a stabilization PR? Or are there some concerns I am missing here. |
The only thing that I miss is the more generic:
But that would need const generics and the split into two parts is by far the most common subcase, so a dedicated and easily recognizable function for that wouldn't hurt. |
That is different from the current |
@internetionals this is possible today using itertools:
I believe we'll add an EDIT: submitted #79659 |
Actually, before we stabilize this, let's dogfood this a bit! I see there are quite a few opportunities to use this function in rust-lang/rust:
Could someone send a PR replacing those usages with |
Hey @matklad, I'd be interested in doing this, but first want to check if it's sensible for a first-time contributor to take this? With checking that it all works, would running through CI be sufficient or you'd want manual testing? |
@Eric-Arellano yup, this would be good first issue, especially given that you can do it running only |
Thanks @matklad! I got it set up and am making progress. FYI: one thing I'm encountering a lot is a difference in API, that let (key, value) = option.split_once('=').unwrap();
let value = if value.is_empty() { None } else { Some(value) }; (As discussed in https://internals.rust-lang.org/t/pre-rfc-none-if-string-is-empty/6742, sounds like we don't want to add sugar to that second line.) Compared to: let mut iter = option.splitn(2, '=');
let key = iter.next().unwrap();
let value = iter.next(); Note that my rewrite is missing an assertion that the key is non-empty. I'm not sure if I should be adding While I do agree with the decision to return Lmk if it would help to put up my WIP so that you can see this diff. |
The following program: fn main() {
println!("{:?}", "".splitn(2, ":").collect::<Vec<_>>());
} Returns the following string: [""] Note how the first element is always here when splitting. If a string is ["", ""] |
@Eric-Arellano I think both versions treat the missing value ( I don't think we should use |
Thank you for pointing that out, @matklad and @xfix. See #79809 - there were several cases where the nuance that |
Dogfood `str_split_once()` Part of rust-lang#74773. Beyond increased clarity, this fixes some instances of a common confusion with how `splitn(2)` behaves: the first element will always be `Some()`, regardless of the delimiter, and even if the value is empty. Given this code: ```rust fn main() { let val = "..."; let mut iter = val.splitn(2, '='); println!("Input: {:?}, first: {:?}, second: {:?}", val, iter.next(), iter.next()); } ``` We get: ``` Input: "no_delimiter", first: Some("no_delimiter"), second: None Input: "k=v", first: Some("k"), second: Some("v") Input: "=", first: Some(""), second: Some("") ``` Using `str_split_once()` makes more clear what happens when the delimiter is not found.
Dogfood `str_split_once()` Part of rust-lang#74773. Beyond increased clarity, this fixes some instances of a common confusion with how `splitn(2)` behaves: the first element will always be `Some()`, regardless of the delimiter, and even if the value is empty. Given this code: ```rust fn main() { let val = "..."; let mut iter = val.splitn(2, '='); println!("Input: {:?}, first: {:?}, second: {:?}", val, iter.next(), iter.next()); } ``` We get: ``` Input: "no_delimiter", first: Some("no_delimiter"), second: None Input: "k=v", first: Some("k"), second: Some("v") Input: "=", first: Some(""), second: Some("") ``` Using `str_split_once()` makes more clear what happens when the delimiter is not found.
This function is very useful for writing simple ad-hoc parsers, and I find its current signature perfect for that. I looked at the other suggestions but returning a single enum which shows whether or not the separator was found seems clearly the right choice to me. I briefly considered the minor variation (I vaguely remember there being some precedent for this kind of API where the TLDR: I think the current signature is the right one and would very much like this to be stabilized. |
Please do! I missed this somehow, yikes. |
No sure if this is the right place to ask, but shouldn't we have |
PR is up. |
TOCTOU: stabilization rules changed a bit in the meantime, see https://github.com/rust-lang/rustc-dev-guide/pull/1036/files#diff-c41e6c5c33d6960efeff23f2f54c1d948ea73f64d604deda521e93ae0b4bd79bR75? @m-ou-se could you kick an FCP here based on this excellent summary? |
Has this question been resolved yet? |
I would think so (by #74773 (comment)):
|
@rfcbot merge |
Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. The RFC will be merged soon. |
…=m-ou-se Stabilize str_split_once Closes rust-lang#74773
…=m-ou-se Stabilize str_split_once Closes rust-lang#74773
…=m-ou-se Stabilize str_split_once Closes rust-lang#74773
@rvolgers wrote on 11 Dec 2020:
Perhaps you meant OsString::into_string? |
Feature gate:
#![feature(str_split_once)]
Public API
Steps / History
Unresolved Questions
(&str, &str, &str)
)? (str::split_once is underpowered #76512)The text was updated successfully, but these errors were encountered: