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

Add method `String::insert_str` #34771

Merged
merged 1 commit into from Jul 22, 2016

Conversation

Projects
None yet
9 participants
@murarth
Copy link
Contributor

murarth commented Jul 11, 2016

No description provided.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jul 11, 2016

r? @brson

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

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jul 11, 2016

The functionality makes sense to me. Is there really no easy way to do this today? cc @rust-lang/libs

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jul 11, 2016

@murarth Can you please change the stability attribute to 'unstable'? New features mostly don't get stabilized immediately.

@murarth

This comment has been minimized.

Copy link
Contributor Author

murarth commented Jul 11, 2016

This is pretty common functionality whose absence I thought was a bit strange. The signature of insert_str mirrors insert and the name goes along with the push/push_str methods.

This seems to me like a sufficiently small addition not to warrant an RFC, so I thought I'd go ahead and submit it.

The #[stable] attribute on the new method was just to get it to pass make check on my machine. I will gladly change it if that's deemed appropriate.

@murarth

This comment has been minimized.

Copy link
Contributor Author

murarth commented Jul 11, 2016

@brson: Sure, I just didn't know what to put for issue or whether the compiler would accept it without that.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jul 11, 2016

@murarth "0" is fine for now. If this PR is accepted then before it's merged you'll need to file a 'tracking' issue on the issue tracker to fill in the real number.

@murarth

This comment has been minimized.

Copy link
Contributor Author

murarth commented Jul 11, 2016

@brson: Okay. It's unstable now.

@BurntSushi

This comment has been minimized.

Copy link
Member

BurntSushi commented Jul 11, 2016

This seems fine to me. I agree that it's small enough that we probably wouldn't need an RFC for it.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jul 12, 2016

LGTM

@ollie27

This comment has been minimized.

Copy link
Contributor

ollie27 commented Jul 12, 2016

This is covered by String::splice rust-lang/rfcs#1432.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 12, 2016

Ah yeah I was also under the impression that this was going to be functionality implemented through splice that @ollie27 linked to. Perhaps the PR to implement could be revived?

@murarth

This comment has been minimized.

Copy link
Contributor Author

murarth commented Jul 12, 2016

I wasn't aware of that RFC. Even so, splice looks to be a slightly different kind of operation. If I understand correctly, then inserting a string without replacing would look something like a.splice(idx..idx, b), which looks a bit odd.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 12, 2016

Yeah it's true that does look a bit odd! We might still have API room to add a function like this, and to me it seems to fit all existing conventions so I'd also be fine layering this on top (although it'd probably use splice as an implementation detail eventually)

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jul 13, 2016

I'm likewise happy having this as a convenience on top of splice.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 19, 2016

Discussed during libs triage today the decision was to merge, thanks for the PR @murarth!

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 19, 2016

📌 Commit 0bcf64c has been approved by alexcrichton

bors added a commit that referenced this pull request Jul 19, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 19, 2016

⌛️ Testing commit 0bcf64c with merge 67f0941...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 19, 2016

💔 Test failed - auto-win-msvc-64-opt

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 19, 2016

@bors: retry

On Tue, Jul 19, 2016 at 12:46 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-opt
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt/builds/4936


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#34771 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95NRWv0XHT0MiKu8iESzlmdl58ApFks5qXSmMgaJpZM4JJ1x-
.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 20, 2016

⌛️ Testing commit 0bcf64c with merge 482a03c...

bors added a commit that referenced this pull request Jul 20, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 20, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 20, 2016

@bors: retry

On Tue, Jul 19, 2016 at 9:19 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt-rustbuild/builds/1822


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#34771 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95N5Rz03tPpQZoHZrYQ9GlVcecAlDks5qXaHKgaJpZM4JJ1x-
.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 21, 2016

⌛️ Testing commit 0bcf64c with merge 44b6d38...

bors added a commit that referenced this pull request Jul 21, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 21, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 21, 2016

@bors: retry

On Wed, Jul 20, 2016 at 5:40 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/1861


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#34771 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95EOEDk87Sfz4yXMXNMkK9T3hotQGks5qXr_2gaJpZM4JJ1x-
.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 21, 2016

⌛️ Testing commit 0bcf64c with merge 6722228...

bors added a commit that referenced this pull request Jul 21, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 21, 2016

💔 Test failed - auto-linux-64-debug-opt

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 21, 2016

@bors: retry

sorry for the number of retries...

On Thu, Jul 21, 2016 at 12:21 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-debug-opt
https://buildbot.rust-lang.org/builders/auto-linux-64-debug-opt/builds/3189


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#34771 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95I7ulfxkYyExSanlqzqda1L0nfIvks5qX8a_gaJpZM4JJ1x-
.

@brson brson removed the I-nominated label Jul 21, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 22, 2016

⌛️ Testing commit 0bcf64c with merge d15e265...

bors added a commit that referenced this pull request Jul 22, 2016

@bors bors merged commit 0bcf64c into rust-lang:master Jul 22, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@murarth murarth deleted the murarth:string-insert-str branch Jul 22, 2016

@murarth

This comment has been minimized.

Copy link
Contributor Author

murarth commented Aug 9, 2016

Just curious, will the libs team create a tracking issue for this at some time or am I supposed to file one myself?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 9, 2016

Oh oops! Thanks for the reminder @murarth! This actually should have held off on landing until a tracking issue was made, but oh well.

If you want to open a tracking issue and send a PR updating the reference here I'll r+ and tag appropriately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.