Helper method on Any to allow an any to be unpacked more easily

[jskeet]

We already have the TypeRegistry abstraction for JSON parsing, so it lends itself well to this.

Note that this is much more useful than it would have been before C# gained pattern matching support: it's easy to imagine a switch statement/expression using pattern matching with the result of this, with cases for a set of known message types, for example.

[jskeet]

Note: this is very tentative at the moment. It works, but I'll need to check with the protobuf team whether this is a pattern used elsewhere, and whether there's a different name I should use for consistency, etc.

[jtattermusch]

Is this ready for review? Please let me know once it is. I think an example to demonstrate the use case would be useful.

[jskeet]

Yes, it's ready for review, in terms of the code - just not ready in terms of needing to check it with Protobuf team members. Will write some sample code here as a comment after a meeting...

[jskeet]

Okay, here's a concrete example, based on [error details](https://github.com/googleapis/googleapis/blob/master/google/rpc/error_details.proto( and status:

var status = /* obtain the status from an RpcException */;
var typeRegistry = /* a type registry including error_details.proto */;
foreach (var detail in status.Details)
{
    switch (detail.UnpackMessage(typeRegistry))
    {
        case ErrorInfo errorInfo:
            Console.WriteLine($"Error info: {errorInfo.Domain} / {errorInfo.Reason}");
            Console.WriteLine($"Metadata keys: {string.Join(", ", errorInfo.Metadata.Keys)}");
            break;
        case LocalizedMessage lm:
            Console.WriteLine($"Localized message ({lm.locale}): {lm.Message}");
            break;
        case Help help:
            Console.WriteLine($"Help, first link: {help.Links.FirstOrDefault().Url}");
            break;
        case BadRequest badReq:
            Console.WriteLine($"Bad request; broken fields: {string.Join(", ", badReq.FieldViolations.Select(fv => fv.Field))}");
            break;
        case IMessage unhandled:
            Console.WriteLine($"Known but unhandled message: {unhandled}");
            break;
        default:
            Console.WriteLine($"Unknown detail type URL: {detail.TypeUrl}");
            break;
    }
}

(This code hasn't been compiled or tested, so may have some typos, but should get across the utility of not having to call Any.TryUnpack lots of times.)

[jtattermusch]

Seems useful. LGTM for the C# code. Please check the general idea with the protobuf team.

[JamesNK]

Why UnpackMessage instead of Unpack?

[JamesNK]

(I'm guessing having Unpack overloads with different return types could be confusing)

[jskeet]

Precisely your guess. Although that doesn't mean I'm unwilling to discuss it :)

[JamesNK]

I don't know how much president there is for different return types in overloads from .NET APIs or ASP.NET Core APIs. Is there is an example of a popular API that has different return types? I can't think of one off the top of my head.

[jskeet]

There's some precedent for this, in terms of Enum.Parse vs Enum.Parse<TEnum>. But it's relatively rare.

[JamesNK]

IMO I'd name them the same. They both unpack a message, just the arguments to do it (generic vs TypeRegistry) are different.

[mkruskal-google]

+1 to Unpack. Both of these methods "unpack a message" as James said, so I find it more confusing for only one of them to be named "UnpackMessage". I supposed something like "UnpackFromTypeRegistry" would work too, but that seems a bit verbose

[jskeet]

Renamed to Unpack.

[amanda-tarafa]

LGTM

[amanda-tarafa]

Aren't these two the same?

[mkruskal-google]

+1, isn't this redundant?

[jskeet]

Yup. I think when I originally wrote the test I did have some reason in mind, but it's gone now :)

[mkruskal-google]

+1 to Unpack. Both of these methods "unpack a message" as James said, so I find it more confusing for only one of them to be named "UnpackMessage". I supposed something like "UnpackFromTypeRegistry" would work too, but that seems a bit verbose

[mkruskal-google]

+1, isn't this redundant?

[mkruskal-google]

This LGTM, but let's go through a lightweight Tier 2 review first since this is a public API change

[jskeet]

This LGTM, but let's go through a lightweight Tier 2 review first since this is a public API change

Yup. Will do that on Tuesday. (UK has public holidays Friday/Monday.)