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 String::splice #44643

Closed
dtolnay opened this Issue Sep 17, 2017 · 9 comments

Comments

Projects
None yet
5 participants
@dtolnay
Member

dtolnay commented Sep 17, 2017

RFC: rust-lang/rfcs#1432

This was originally tracked in #32310 as a combination of Vec::splice and String::splice, but the libs team decided to stabilize Vec::splice while keeping String::splice unstable and changing its signature in #44038.

The new signature has been implemented in #44044 but needs to go through FCP.

impl String {
    pub fn splice<R>(&mut self, range: R, replace_with: &str)
        where R: RangeArgument<usize>;
}
@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Nov 15, 2017

Contributor

Ping @rust-lang/libs. FCP to stabilize?

Contributor

SimonSapin commented Nov 15, 2017

Ping @rust-lang/libs. FCP to stabilize?

@dtolnay

This comment has been minimized.

Show comment
Hide comment
@dtolnay

dtolnay Nov 15, 2017

Member

I pushed for changing the signature so I will take responsibility for it. 😸

@rfcbot fcp merge

Member

dtolnay commented Nov 15, 2017

I pushed for changing the signature so I will take responsibility for it. 😸

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Nov 15, 2017

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, 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 commented Nov 15, 2017

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, 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.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Nov 15, 2017

Member

As I mentioned in #32310 (comment), I'm 👎 to this if the method is called splice() as it does not return the deleted part, unlike Vec::splice(). Calling it replace_slice() seems more appropriate.

(

Other languages either provides the IndexAssign operator x[range] = replace_with (e.g. Ruby, Perl), doesn't have a method for this (e.g. C#), or doesn't have mutable string (e.g. Python, JavaScript, Go).

We already used replace() for something else, so we can't name it replace() unless we have RangeArgument<usize> extends Pattern.

)

Member

kennytm commented Nov 15, 2017

As I mentioned in #32310 (comment), I'm 👎 to this if the method is called splice() as it does not return the deleted part, unlike Vec::splice(). Calling it replace_slice() seems more appropriate.

(

Other languages either provides the IndexAssign operator x[range] = replace_with (e.g. Ruby, Perl), doesn't have a method for this (e.g. C#), or doesn't have mutable string (e.g. Python, JavaScript, Go).

We already used replace() for something else, so we can't name it replace() unless we have RangeArgument<usize> extends Pattern.

)

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Nov 15, 2017

Contributor

replace_range?

Contributor

SimonSapin commented Nov 15, 2017

replace_range?

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Nov 21, 2017

Member

I like replace_range.

Member

sfackler commented Nov 21, 2017

I like replace_range.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Nov 22, 2017

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

rfcbot commented Nov 22, 2017

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

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Dec 2, 2017

The final comment period is now complete.

rfcbot commented Dec 2, 2017

The final comment period is now complete.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Mar 17, 2018

Contributor

Help wanted: stabilization PR that also renames to replace_range.

Contributor

SimonSapin commented Mar 17, 2018

Help wanted: stabilization PR that also renames to replace_range.

tmccombs added a commit to tmccombs/rust that referenced this issue Apr 2, 2018

kennytm added a commit to kennytm/rust that referenced this issue Apr 4, 2018

Rollup merge of #49577 - tmccombs:string-splice-stabilize, r=TimNN
Stabilize String::replace_range

Fixes #44643
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment