OneOf icon indicating copy to clipboard operation
OneOf copied to clipboard

Implements `IsX` and `AsX`

Open mniak opened this issue 3 years ago • 9 comments

Closes #93

mniak avatar Sep 09 '21 16:09 mniak

@mcintyre321 i think i'm done with the first part here

mniak avatar Sep 09 '21 19:09 mniak

These examples are making me think that it's not going to be possible to come up with a default way to render the name elegantly every time.

It might be better to leave the default naming strategy as is (e.g. IsT0, IsT1, etc).

Then have the generator attribute take an optional string array where the names can be overridden and set explicitly.

On Fri, 10 Sep 2021, 13:47 Andre Soares, @.***> wrote:

@.**** commented on this pull request.

In OneOf.SourceGenerator/OneOfGenerator.cs https://github.com/mcintyre321/OneOf/pull/95#discussion_r706151816:

  •    public bool Is{arg.Name} => this.Is{param.Name};
    
  •    public {arg.ToDisplayString()} As{arg.Name} => this.As{param.Name};
    
  •    public bool TryPick{arg.Name}(out {arg.ToDisplayString()} value, out {remainderArgsString} remainder) => this.TryPick{param.Name}(out value, out remainder);
    

Maybe we could try adding the number of type parameters before the type arguments, like:

  • List -> ListOf1String
  • Dictionary<string, int> -> DictionaryOf2StringInt32

for the problematic cases mentioned above:

  • OneOf<OneOf<byte, short, int>, long> -> OneOf2OneOf3ByteShortIntLong
  • OneOf<OneOf<byte, short>, int, long> - OneOf3OneOf2ByteShortIntLong

but it is very difficult to solve all conflicts, since an identifier can only have some characters but we are trying to represent characters that are not present. we must come out with some kind of escape character logic

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mcintyre321/OneOf/pull/95#discussion_r706151816, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACDJ6TVUTCBTK2NEPWNMBLUBH443ANCNFSM5DXR7UJA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

mcintyre321 avatar Sep 10 '21 12:09 mcintyre321

@mcintyre321 yeah, we kind of thought the same thing at the same time. I agree, but would suggest that we have at least a simple/incomplete/defective way shipped by default and if any conflict arise then we just dont render these properties.

And maybe raise a diagnostic warning.

But this default naming strategy could come in a next pull request. The array would be of much help already.

mniak avatar Sep 10 '21 13:09 mniak

The biggest problem I see isn't conflicts, it's that ReallyLongTypeNamesAndGenericsWillBeVeryUgly

On Fri, 10 Sep 2021, 14:01 Andre Soares, @.***> wrote:

@mcintyre321 https://github.com/mcintyre321 yeah, we kind of thought the same thing at the same time. I agree, but would suggest that we had at least a simple incomplete way shipped by default and if any conflict arise then we just dont render these properties.

And maybe raise a diagnostic warning

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mcintyre321/OneOf/pull/95#issuecomment-916886145, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACDJ6TIZ44GJLLLYWRS2PLUBH6SDANCNFSM5DXR7UJA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

mcintyre321 avatar Sep 10 '21 13:09 mcintyre321

If we choose to stay with As/IsT1 we can add XML comments with appropriate descriptions explaining what 0/1/2 stands for eg: image but we will have similar problem because there is no way to reference closed generic type, this is the closest solution: image so in some edge cases it could be very long but it will work.

romfir avatar Sep 10 '21 13:09 romfir

@romfir Is not possible to add this comments, since the classes OneOfBase<T1, T2, ... Tn> are declared in the project OneOf and for that reason are not generated by the source generator

mniak avatar Sep 10 '21 19:09 mniak

@mcintyre321 I removed the default naming strategy and added a parameters to the attribute constructor in order to determine the naming. If the arguments are not passed, the generation of this named properties does not happen.

I have also added some diagnostic errors for special cases

  • when the number names informed on the attribute does not match the number of cases
  • when some name is null

mniak avatar Sep 12 '21 15:09 mniak

I did extensions like this that are super handy ...

public static class OneOfIs
{
    public static bool Is<T>(this IOneOf oneOf, [NotNullWhen(true)] out T? value)
    {
        if (oneOf.Value is T v)
        {
            value = v;
            return true;
        }

        value = default;
        return false;
    }

    public static bool Is<T>(this IOneOf oneOf)
    {
        if (oneOf.Value is T)
        {
            return true;
        }

        return false;
    }
}

smokedlinq avatar Mar 17 '22 20:03 smokedlinq

What is the future of this PR? This feature is the only blocker for us to migrate to OneOf library from a self-written union type.

Ne4to avatar Jun 23 '22 08:06 Ne4to