Path.Join with four arguments

[jnm2]

I wanted to write this:

Path.Join(assetsDirectory, "objects", hash.AsSpan().Slice(0, 2), hash)

This is the workaround:

Path.Join(assetsDirectory, "objects", Path.Join(hash.AsSpan().Slice(0, 2), hash))

Path.Combine goes up to four; why is this API different?

API Summary

We didn't propose a 4 method overload originally as TryJoin allowed writing low-allocating versions for 4+ paths.

As we already have the same helper code for both Combine/Join, there is no reason not to expose the 4 method overloads for Join. TryJoin isn't as straight-forward and can be composed with multiple calls without extra allocation cost- as such we'll leave at 3 for now.

API Signature

namespace System.IO
{
    public static class Path
    {
        public static string Join(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3, ReadOnlySpan<char> path4);
        public static string Join(string path1, string path2, string path3, string path4);
    }
}

[danmoseley]

@JeremyKuhne any objection? It would obviously be easy to add

[JeremyKuhne]

It's simply a matter of diminishing returns vs. additional complexity. The idea was that for 4+ you could compose yourself using TryJoin if allocation was critical. I'm fine with adding this one as we already have the helper.

https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/IO/Path.cs#L607

[jbhensley]

If I'm looking at this correct, there will be two PRs. One in corefx for the API and one in coreclr for the actual implementation?

I assume we'll want to add overloads for TryJoin and Join(string...) to maintain parity. How about this:

public static string Join(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3, ReadOnlySpan<char> path4) { throw null; }
public static bool TryJoin(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3, ReadOnlySpan<char> path4, Span<char> destination, out int charsWritten) { throw null; }
public static string Join(string path1, string path2, string path3, string path4) { throw null; }

[JeremyKuhne]

@jbhensley Good point on the string one. I'd rather not do the TryJoin unless we really demonstrate a demand for it. It isn't as straight-forward as it might seem at first as it has to not copy data if it can't fit everything.

[jbhensley]

Totally your call. Looking at the code for TryJoin with three, it doesn't look like it would be too difficult to extend out to four. Don't know if there is any demand, but things feel a bit "off" if Join goes to four and TryJoin goes to three. Just my personal take.

[JeremyKuhne]

It isn't horrible to implement for sure, but I'm trying to be a little cautious by default. We'll see what the API review has to say about it- if the general consensus is to match I'm fine with it.

[jbhensley]

I'd like to take this on if the API gets approved in whichever form. Looks pretty straightforward.

[JeremyKuhne]

@jbhensley sure! @karelz, can you get him set up so we can assign the issue to him?

[karelz]

@jbhensley invite sent. Ping us once you accept it - we can then assign it to you. It will auto-subscribe you to all repo notifications (500+ per day). We recommend to switch repo to Not-Watching which will reduce notifications to just explicit subscriptions, mentions and assignments.

[jbhensley]

@karelz Invite accepted

[jbhensley]

@karelz @JeremyKuhne Let me know if there is anything else you need me to do. I'm sure this is not a very high priority item.

[karelz]

Sorry, I was at MSIgnite, not on email/GitHub. @JeremyKuhne can you push on the API review next Tuesday please?

[JeremyKuhne]

can you push on the API review next Tuesday please?

As soon as we get back to the backlog I'll be online for it.

[terrajobst]

This makes sense to me. We'd also match the pattern of Path.Combine() which has specialized overloads for up to four arguments as well (and then falls back to params).

[terrajobst]

Looks good.

namespace System.IO
{
    public static class Path
    {
        public static string Join(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3, ReadOnlySpan<char> path4);
        public static string Join(string path1, string path2, string path3, string path4);

        // For consistency we should also add this one:
        public static string Join(params string[] paths);
    }
}

@JeremyKuhne Any objections to adding the params one too? We want people to move from Path.Combine() to this one and passing more than four paths seems reasonable. For spans we're out of luck for know, though.

[JeremyKuhne]

Any objections to adding the params one too?

Not really.

[Anipik]

@jbhensley can you are planning to take this up ?

[Anipik]

Tests and api are both merged.