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

Stabilize split_inclusive #77858

Merged
merged 6 commits into from
Jan 13, 2021
Merged

Stabilize split_inclusive #77858

merged 6 commits into from
Jan 13, 2021

Conversation

ijackson
Copy link
Contributor

Contents of this MR

This stabilises:

  • slice::split_inclusive
  • slice::split_inclusive_mut
  • str::split_inclusive

Closes #72360.

A possible concern

The proliferation of split_* methods is not particularly pretty. The existence of split_inclusive seems to invite the addition of rsplit_inclusive, splitn_inclusive, etc. We could instead have a more general API, along these kinds of lines maybe:

   pub fn split_generic('a,P,H>(&'a self, pat: P, how: H) -> ...
       where P: Pattern
       where H: SplitHow;

   pub fn split_generic_mut('a,P,H>(&'a mut self, pat: P, how: H) -> ...
       where P: Pattern
       where H: SplitHow;

   trait SplitHow {
       fn reverse(&self) -> bool;
       fn inclusive -> bool;
       fn limit(&self) -> Option<usize>;
   }

   pub struct SplitFwd;
   ...
   pub struct SplitRevInclN(pub usize);

But maybe that is worse.

Let us defer that?

This seems like a can of worms. I think we can defer opening it now; if and when we have something more general, these two methods can become convenience aliases. But I thought I would mention it so the lang API team can consider it and have an opinion.

@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 @withoutboats (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. Due to 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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2020
@jonas-schievink jonas-schievink added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. relnotes Marks issues that should be documented in the release notes of the next release. labels Oct 12, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Oct 30, 2020

Thanks @ijackson! We do have quite a lot of split methods on str, but I don't think these methods make that situation any worse than it currently is, and I don't think solutions to that proliferation require any new tools.

I think an API that might untangle things in the future could be to introduce something like a Splitter type that pulled the various ways of surfacing substrings and patterns off the str type itself:

// do:
s.splitter(pat).inclusive()
s.rsplitter(pat).inclusive()

// instead of:
s.split_inclusive(pat)
s.rsplit_inclusive(pat)

If we did want to do something like that and deprecate methods on str I don't think that would introduce any new problems of dealing with discoverability amongst a growing number of deprecated APIs than we already have.

@KodrAus
Copy link
Contributor

KodrAus commented Oct 30, 2020

These seem like reasonable additions to our split API to me that solve another set of use-cases where you want to keep the split pattern on the end of each substring.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Oct 30, 2020

Team member @KodrAus 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 30, 2020
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 20, 2020
@ijackson ijackson changed the title Stablize split_inclusive Stabilize split_inclusive Dec 1, 2020
@m-ou-se m-ou-se added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 8, 2020
@bors
Copy link
Contributor

bors commented Dec 31, 2020

☔ The latest upstream changes (presumably #79895) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 31, 2020
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Dec 31, 2020
@rfcbot
Copy link

rfcbot commented Dec 31, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
    Checking rustc-rayon v0.3.0
    Checking tempfile v3.1.0
    Checking regex v1.3.9
    Checking rustdoc v0.0.0 (/checkout/src/librustdoc)
error: the feature `split_inclusive` has been stable since 1.49.0 and no longer requires an attribute to enable
   |
18 | #![feature(split_inclusive)]
   |            ^^^^^^^^^^^^^^^
   |
   |
   = note: `-D stable-features` implied by `-D warnings`
error: aborting due to previous error

error: could not compile `rustdoc`

@ijackson
Copy link
Contributor Author

ijackson commented Jan 4, 2021

@rustbot modify labels -S-waiting-on-author

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 4, 2021
@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Jan 10, 2021
@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jan 10, 2021
@rfcbot
Copy link

rfcbot commented Jan 10, 2021

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.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jan 10, 2021
@KodrAus
Copy link
Contributor

KodrAus commented Jan 13, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jan 13, 2021

📌 Commit 5584224 has been approved by KodrAus

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 13, 2021
@bors
Copy link
Contributor

bors commented Jan 13, 2021

⌛ Testing commit 5584224 with merge 9f3998b...

@bors
Copy link
Contributor

bors commented Jan 13, 2021

☀️ Test successful - checks-actions
Approved by: KodrAus
Pushing 9f3998b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 13, 2021
@bors bors merged commit 9f3998b into rust-lang:master Jan 13, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 13, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jan 14, 2021
@jyn514
Copy link
Member

jyn514 commented Mar 22, 2021

Shouldn't since version be 1.51?

This was fixed before merging: 5584224

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue for split_inclusive