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
Merged

Add method String::insert_str #34771

merged 1 commit into from
Jul 22, 2016

Conversation

murarth
Copy link
Contributor

@murarth murarth commented Jul 11, 2016

No description provided.

@rust-highfive
Copy link
Collaborator

r? @brson

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

@brson
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 brson added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 11, 2016
@brson
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
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
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
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
Copy link
Contributor Author

murarth commented Jul 11, 2016

@brson: Okay. It's unstable now.

@BurntSushi
Copy link
Member

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

@sfackler
Copy link
Member

LGTM

@ollie27
Copy link
Member

ollie27 commented Jul 12, 2016

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

@alexcrichton
Copy link
Member

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
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
Copy link
Member

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
Copy link
Member

aturon commented Jul 13, 2016

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

@alexcrichton
Copy link
Member

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

@bors: r+

@bors
Copy link
Contributor

bors commented Jul 19, 2016

📌 Commit 0bcf64c has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jul 19, 2016

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

@bors
Copy link
Contributor

bors commented Jul 19, 2016

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

@alexcrichton
Copy link
Member

@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
Copy link
Contributor

bors commented Jul 20, 2016

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

@bors
Copy link
Contributor

bors commented Jul 20, 2016

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

@alexcrichton
Copy link
Member

@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
Copy link
Contributor

bors commented Jul 21, 2016

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

@bors
Copy link
Contributor

bors commented Jul 21, 2016

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

@alexcrichton
Copy link
Member

@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
Copy link
Contributor

bors commented Jul 21, 2016

⌛ Testing commit 0bcf64c with merge 6722228...

@bors
Copy link
Contributor

bors commented Jul 21, 2016

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

@alexcrichton
Copy link
Member

@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
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
@murarth murarth deleted the string-insert-str branch July 22, 2016 16:14
@murarth
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
Copy link
Member

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
Labels
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.

9 participants