mcintyre321 / OneOf

Easy to use F#-like ~discriminated~ unions for C# with exhaustive compile time matching
MIT License
3.5k stars 164 forks source link

Source generator should implement `IsX` and `AsX` properties #93

Open mniak opened 3 years ago

mniak commented 3 years ago

Currently, if we want to check if an object is of a specific type, we need to use properties with the generic names like IsT0, IsT1, etc.

Take the following class as an example:

[GenerateOneOf]
public partial class Result : OneOfBase<Result.Success, Result.Failure> {
  public class Success {}
  public class Failure {}
}

If i want to check if the object is of type Success, I need to either

  1. Use IsT0. not very meaningful
  2. Implement public bool IsSuccess => IsT0

My proposal is to also generate the property IsSuccess as described in alternative 2.

What if the properties conflict with some existing member? We could have a parameter in the attribute to indicate if this feature should be disabled, like:

[GenerateOneOf(DisableNamedPropertiesGeneration=true)]
mcintyre321 commented 3 years ago

Rather than have the GenerateOneOf(DisableNamedPropertiesGeneration=true), the generator could include a cleaned up version of each types namespace when conflicts are

On Thu, 9 Sep 2021, 17:07 Harry McIntyre, @.***> wrote:

If you could extend this to cover the TryPickT1 methods then that would be very cool indeed.

On Thu, 9 Sep 2021, 15:58 Andre Soares, @.***> wrote:

Currently, if we want to check if an object is of a specific type, we need to use properties with the generic names like IsT0, IsT1, etc.

Take the following class as an example:

[GenerateOneOf]public partial class Result : OneOfBase<Result.Success, Result.Failure> { public class Success {} public class Failure {} }

If i want to check if the object is of type Success, I need to either

  1. Use IsT0. not very meaningful
  2. Implement public bool IsSuccess => IsT0

My proposal is to also generate the property IsSuccess as described in alternative 2.

What if the properties conflict with some existing member? We could have a parameter in the attribute to indicate if this feature should be disabled, like:

[GenerateOneOf(DisableNamedPropertiesGeneration=true)]

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mcintyre321/OneOf/issues/93, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACDJ6XA5H63C2QOB72BJTLUBDDRPANCNFSM5DXKYIMA . 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 commented 3 years ago

If you could extend this to cover the TryPickT1 methods then that would be very cool indeed.

On Thu, 9 Sep 2021, 15:58 Andre Soares, @.***> wrote:

Currently, if we want to check if an object is of a specific type, we need to use properties with the generic names like IsT0, IsT1, etc.

Take the following class as an example:

[GenerateOneOf]public partial class Result : OneOfBase<Result.Success, Result.Failure> { public class Success {} public class Failure {} }

If i want to check if the object is of type Success, I need to either

  1. Use IsT0. not very meaningful
  2. Implement public bool IsSuccess => IsT0

My proposal is to also generate the property IsSuccess as described in alternative 2.

What if the properties conflict with some existing member? We could have a parameter in the attribute to indicate if this feature should be disabled, like:

[GenerateOneOf(DisableNamedPropertiesGeneration=true)]

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mcintyre321/OneOf/issues/93, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACDJ6XA5H63C2QOB72BJTLUBDDRPANCNFSM5DXKYIMA . 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.

mniak commented 3 years ago

@mcintyre321 I have implemented the TryPickX methods like you suggested.

mniak commented 3 years ago

Regarding to not having the attribute parameter, I think that we should think more, since there are scenarions in which even the name+namespace would not be enough to dismiss the ambiguity.

like

public class MyClass : OneOfBase<string, string> {
  public bool IsString => this.IsT0;
  public bool IsString => this.IsT1; // ambiguous
}

I known that using this library to alternate between two integers does not appear to make sense, but it could happen. Maybe i could raise a disgnostic error, but I dont know if this would be desirable

mcintyre321 commented 3 years ago

I'm sometimes tempted to change the methods in the base to bool Is<T>() where T:T1 which would possibly make this PR unnecessary? There are pros and cons. I guess the language would feel with namespaces, and ambiguous type names, but it makes it tricky if one of the union types inherits from another...

On Thu, 9 Sep 2021, 18:52 Andre Soares, @.***> wrote:

Regarding to not having the attribute parameter, I think that we should think more, since there are scenarions in which even the name+namespace would not be enough to dismiss the ambiguity.

like

public class MyClass : OneOfBase<string, string> { public bool IsString => this.IsT0; public bool IsString => this.IsT1; // ambiguous }

I known that using this library to alternate between two integers does not appear to make sense, but it could happen. Maybe i could raise a disgnostic error, but I dont know if this would be desirable

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mcintyre321/OneOf/issues/93#issuecomment-916312715, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACDJ6TG4WUVU6SFA4ROO43UBDX7PANCNFSM5DXKYIMA . 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.

jvmlet commented 1 year ago

Another suggestion is to try to follow naming convention Name0OrName1OrName2OrNameN :

For example

[GenerateOneOf(pattern="custom_regex_pattern")] // optional pattern defaults to `((?![Or]).)+`
public partial class ResultOrErrorOrNothing<TResult> : OneOfBase<TResult, Exception,None> {
}

produces shortcuts

{
public bool IsResult => IsT0;
public bool IsError => IsT1;
public bool IsNothing => IsT2;
}
jvmlet commented 1 year ago

More options :

@mcintyre321 , if you accept one of the proposed three , I can PR.

cremor commented 1 year ago

What is the status here? @mniak closed PR #95 without a comment.

I think this would be a very important addition to get a more readable code when using this library.

The idea of @mcintyre321 to change the implementation to bool Is<T>() where T:T0 in OneOfBase would help a bit, but real property names are still better:

// Ugly
if (result.IsT0) { }
// Better
if (result.Is<Success>()) { }
// Best
if (result.IsSuccess) { }

The name conflict would also affect the bool Is<T>() where T:T0 solution (it wouldn't work if multiple type parameters have the same type).

And I think this should be expanded to all members that contain T0 in their name:

mcintyre321 commented 1 year ago

result.Is<Success>() is superior in some regards as you can use full namespaces or aliases if the names overlap

cremor commented 1 year ago

That might be true, but it will not help in the case when the same type is used multiple times. The generated property names could handle both cases with some convention (e.g. just adding a number suffix).

In my opinion both cases should never happen in good code, but who knows 😉