Rename SliceConcatExt::connect to join #26957
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
I broke this out into two separate commits to make it easier to code review. The first commit is the rename and the second changes all of the call sites. If it's better to squash them together before merging them, I'd be happy to do that too. I tried to follow the RFC but if I messed something up, please let me know. Thanks! |
| /// assert_eq!(["hello", "world"].connect(" "), "hello world"); | ||
| /// ``` | ||
| #[stable(feature = "rust1", since = "1.0.0")] | ||
| #[deprecated(since = "1.3.0", reason = "renamed to join")] | ||
| fn connect(&self, sep: &T) -> Self::Output; |
There was a problem hiding this comment.
Probably best to just have this default to calling join so you don't need multiple impls.
There was a problem hiding this comment.
Isn't it backwards-incompatible to do anything other than having join default to calling connect?
There was a problem hiding this comment.
Well, he even had to fix libcollections/str.rs, so it's clearly not backward compatible.
There was a problem hiding this comment.
The trait is marked unstable but the individual functions are marked as stable. What does that mean in terms of backward-compatibility? I tried to make my changes backwards compatible for users of the trait but I hadn't considered implementors.
There was a problem hiding this comment.
@eddyb All implementors must have given an impl of connect, so this default will never apply unless you update your impl to not use connect.
There was a problem hiding this comment.
Without a default on join, no user impls will compile after this change. But if the trait is unstable... It's probably fine.
There was a problem hiding this comment.
It seems to me that providing a default on one of the functions is a good idea. I'm just not sure which one.
If we:
- default
connectto calljoin: then this will break any existing user implementations. However the trait is currently marked unstable. Is breaking backwards compatibility ok then? - default
jointo callconnect: then existing user implementations will continue working. However any future users will have to implement a deprecated method which doesn't seem obvious or desirable. Also, will that generate warnings on their implementations?
There was a problem hiding this comment.
Oh yes. Sorry I saw that this PR was breaking all implementations anyway,
so I immediately paged out "having to implement join" as a breaking change.
:)
Longterm I think connect-defaults-to-join is the right one. Regardless,
sliceconcatext isn't really something that's meant to be impl'd... it's
just a hack we use internally, I think.
On Sat, Jul 11, 2015 at 10:45 AM, Eduard Burtescu notifications@github.com
wrote:
In src/libcollections/slice.rs
#26957 (comment):/// assert_eq!(["hello", "world"].connect(" "), "hello world"); /// ``` #[stable(feature = "rust1", since = "1.0.0")]
- #[deprecated(since = "1.3.0", reason = "renamed to join")]
fn connect(&self, sep: &T) -> Self::Output;Without a default on join, no user impls will compile after this change.
But if the trait is unstable... It's probably fine.—
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/26957/files#r34414816.
There was a problem hiding this comment.
Ok. I'll change connect to default to join which should address the remaining nits. I'll push that change up in a minute. Should I squash everything together or leave the rename and connect-to-join changes in two separate commits?
There was a problem hiding this comment.
Having a seperate "rename" and "fallout" commit seems reasonable.
On Sat, Jul 11, 2015 at 1:42 PM, Wesley Wiser notifications@github.com
wrote:
In src/libcollections/slice.rs
#26957 (comment):/// assert_eq!(["hello", "world"].connect(" "), "hello world"); /// ``` #[stable(feature = "rust1", since = "1.0.0")]
- #[deprecated(since = "1.3.0", reason = "renamed to join")]
fn connect(&self, sep: &T) -> Self::Output;Ok. I'll change connect to default to join which should address the
remaining nits. I'll push that change up in a minute. Should I squash
everything together or leave the rename and connect-to-join changes in
two separate commits?—
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/26957/files#r34416058.
|
r=me with nits |
bc03b6d to
ed472c8
Compare
|
I'd like to hold off on r+'ing this just yet as I'd also like to run this through crater. This is a change to a pretty core type, so it may have unintended impact that we weren't aware of ahead of time. |
|
I'm currently running a crater run. |
|
Ok, here's the regression report. Looks like a few crates have failed because of Thanks @wesleywiser! |
|
I was going to squash the code review feedback before it got merged. The On Sun, Jul 12, 2015, 6:06 PM bors notifications@github.com wrote:
|
|
If this bounces then it's fine to squash, but while bors is testing the branch shouldn't be force pushed. Not much harm either way :) |
|
Is there any way we could move this method to be inherent on slices? The rename is a good opportunity. |
On the functionality side, morestack is gone (rust-lang/rust#27338), and rustrt_native is gone (rust-lang/rust#27176). On the implementation side, connect has been renamed to join (rust-lang/rust#26957).
Fixes #26900