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

Let str::replace take a pattern #29498

Merged
merged 2 commits into from
Jan 13, 2016
Merged

Conversation

wthrowe
Copy link
Contributor

@wthrowe wthrowe commented Nov 1, 2015

It appears this was left out of RFC rust-lang/rfcs#528 because it might be useful to
also generalize the second argument in some way. That doesn't seem to
prevent generalizing the first argument now, however.

This is a [breaking-change] because it could cause type-inference to
fail where it previously succeeded.

Also update docs for a few other methods that still referred to &str instead of patterns.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

let data = "abcdαβγδabcdαβγδ";
assert_eq!(data.replace("dαβ", "😺😺😺"), "abc😺😺😺γδabc😺😺😺γδ");
assert_eq!(data.replace('γ', "😺😺😺"), "abcdαβ😺😺😺δabcdαβ😺😺😺δ");
assert_eq!(data.replace(&['a', 'γ'], "😺😺😺"), "😺😺😺bcdαβ😺😺😺δ😺😺😺bcdαβ😺😺😺δ");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cause of travis failure. Rust can’t auto-deref this array into closure because there’s nothing that explicitly requests a slice.

@wthrowe
Copy link
Contributor Author

wthrowe commented Nov 1, 2015

Sorry about that. Probably fixed now. (I can't run tests locally at the moment because of #29492.)

@bluss
Copy link
Member

bluss commented Nov 1, 2015

With #26870 (Default Type Parameter Fallback) enabled by default, this could avoid being breaking. Not sure if it's worth it to wait.

Maybe there's a case where it's breaking anyway, but basically type param fallback “opens the floodgates” to generalizing API.

@alexcrichton
Copy link
Member

This seems good to me, although I would like to have a crater run ahead of time with this to ensure we don't have much unintended breakage. If we have to wait for #26870 to get turned on by default that's also probably not the end of the world!

@alexcrichton alexcrichton added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 2, 2015
@wthrowe
Copy link
Contributor Author

wthrowe commented Nov 2, 2015

I'm fine with waiting for default type parameter fallback if you prefer. If you decide to wait I'll split the doc fixes into a separate pull request

@bors
Copy link
Contributor

bors commented Dec 1, 2015

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

It appears this was left out of RFC rust-lang#528 because it might be useful to
also generalize the second argument in some way.  That doesn't seem to
prevent generalizing the first argument now, however.

This is a [breaking-change] because it could cause type-inference to
fail where it previously succeeded.
@wthrowe
Copy link
Contributor Author

wthrowe commented Dec 8, 2015

Rebased.

@alexcrichton
Copy link
Member

A crater report reports 4 regressions, but I think they're all spurious and unrelated to this change.

I was thinking about this though and the "most generalized" form of the API here would perhaps be to have the replacement either be &str or perhaps Fn(&str) -> String (or something like that) where you can use the string (which with patterns you don't know quite what matched) to generate the replacement (rather than always have the same replacement).

In some sense though this is getting pretty close to just an API around regexes, however, so we may not want to go too far down that route. Regardless I'm gonna tag this with T-libs so we can talk about it in the libs triage meeting.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 8, 2015
@bluss
Copy link
Member

bluss commented Dec 20, 2015

Ok, default type parameter fallback seems to not be likely to arrive any time soon (unfortunately), so we should continue?

@Gankra Gankra removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jan 6, 2016
@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 6, 2016
@aturon
Copy link
Member

aturon commented Jan 8, 2016

We discussed this in the libs team meeting this week, and are happy to go forward, pending an additional crater run.

@Gankra Gankra added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jan 9, 2016
@alexcrichton
Copy link
Member

Crater reports one regression which is just spurious, so I think this is good to go.

@alexcrichton alexcrichton removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jan 12, 2016
@alexcrichton
Copy link
Member

@bors: r+ 34fe201

@bors
Copy link
Contributor

bors commented Jan 12, 2016

⌛ Testing commit 34fe201 with merge 6fe73d4...

@bors
Copy link
Contributor

bors commented Jan 12, 2016

💔 Test failed - auto-linux-64-opt

@alexcrichton
Copy link
Member

@bors: retry

On Tue, Jan 12, 2016 at 2:49 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-64-opt
http://buildbot.rust-lang.org/builders/auto-linux-64-opt/builds/7639


Reply to this email directly or view it on GitHub
#29498 (comment).

@bors
Copy link
Contributor

bors commented Jan 13, 2016

⌛ Testing commit 34fe201 with merge f40932c...

@bors
Copy link
Contributor

bors commented Jan 13, 2016

💔 Test failed - auto-mac-32-opt

@alexcrichton
Copy link
Member

@bors: retry

On Tuesday, January 12, 2016, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-32-opt
http://buildbot.rust-lang.org/builders/auto-mac-32-opt/builds/7693


Reply to this email directly or view it on GitHub
#29498 (comment).

@bors
Copy link
Contributor

bors commented Jan 13, 2016

⌛ Testing commit 34fe201 with merge 8796e01...

bors added a commit that referenced this pull request Jan 13, 2016
It appears this was left out of RFC rust-lang/rfcs#528 because it might be useful to
also generalize the second argument in some way.  That doesn't seem to
prevent generalizing the first argument now, however.

This is a [breaking-change] because it could cause type-inference to
fail where it previously succeeded.

Also update docs for a few other methods that still referred to `&str` instead of patterns.
@bors bors merged commit 34fe201 into rust-lang:master Jan 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. 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.

10 participants