-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Implement split_inclusive for slice and str #67330
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
r? @SimonSapin |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Hi @Centril. If you’d like help specifically from me feel free to mention me. If you believe highfive’s choice of reviewer is not appropriate (maybe that person is away for a couple weeks?) and need to find another reviewer please consider picking from https://github.com/rust-lang/highfive/blob/master/highfive/configs/rust-lang/rust.json. r? @sfackler |
r? @KodrAus |
(Why reassign a third time so soon after the second one?) |
Ah didn't see that you reassigned. |
☔ The latest upstream changes (presumably #67917) made this pull request unmergeable. Please resolve the merge conflicts. |
I think this makes sense too, and is how I’d expect this to work. |
☔ The latest upstream changes (presumably #68142) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage: @golddranks can you please address the merge conflict? |
Pinging again from triage: @golddranks can you please address the merge conflict? |
@JohnCSimon Ah, sorry! I'll fix the merge conflict now. @KodrAus Thanks for the input. I agree. I'll change the behaviour to that and address that in docs. |
5855eb0
to
c493ab3
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
(Sorry for fixing seemingly stupid errors here, the tests/builds take super long to run locally ATM for some reason, so I'm pushing changes "opportunistically".) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
ping from triage @KodrAus, could you update your review? thanks |
Thanks @golddranks! This looks good to me. Would you like to squash these commits down and I’ll merge in? |
…at includes the matched part in the iterated substrings as a terminator.
…est reverse iteration.
80784eb
to
5c9dc57
Compare
👋 @KodrAus |
@bors r=kodraus |
📌 Commit 5c9dc57 has been approved by |
☀️ Test successful - checks-azure |
Overview
split_inclusive
forslice
andstr
andsplit_inclusive_mut
forslice
split_inclusive
is a substring/subslice splitting iterator that includes the matched part in the iterated substrings as a terminator.split_terminator
function. I updated the examples below.Justification for the API
split
API: it's easy to get the behaviour ofsplit
by mapping a subslicing operation that drops the terminator. On the other hand it's impossible to derive this behaviour fromsplit
without using hacky and brittleunsafe
code. The normal way to achieve this functionality would be implementing the iterator yourself.split_at_mut
. This API provides an ergonomic alternative that plays to the strengths of the iterating capabilities of Rust. (Usingsplit_at_mut
iteratively used to be a real pain before NLL, fortunately the situation is a bit better now.)Discussion items
Does it make sense to mimicsplit_terminator
in that the final empty slice would be left off in case of the string/slice ending with a terminator? It might do, as this use case is naturally geared towards considering the matching part as a terminator instead of a separator.split_terminator
.split_inclusive_mut
for&mut str
?