Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C#] How to handle unconstrained optional generic type arguments #322

Closed
kMutagene opened this issue Jul 11, 2022 · 6 comments
Closed

[C#] How to handle unconstrained optional generic type arguments #322

kMutagene opened this issue Jul 11, 2022 · 6 comments

Comments

@kMutagene
Copy link
Collaborator

kMutagene commented Jul 11, 2022

This is a specific interop problem between the F# core API and the native C# layer.

In short, I'd like to ask for feedback on which type of API surface looks better/more intuitive for C# users and why. I also added a in-depth problem description for the curious further below.

Ask for feedback

Please react with 🚀 for Option 1, or 🎉 for Option 2. Detailed feedback via comments are highly appreciated

As the C# API is shaping out, we encountered some specific friction points between F# and C# that need workarounds. This is not bad per se, but has an influence on the API surface.

In short, we need a Optional<T> type for unconstrained optional parameters that can be both value and reference types.

We now have two ways of using them:

  1. 🚀 use Optional<T> for ALL optional parameters. This would make the signature of Chart.Column look like this:

image

  1. 🎉 only use Optional<T> where needed. In our example, these are the optional arguments Base, Width, and Text. This makes the signature more concise, but makes it less obvious that there is no special ceremony of setting these values. This would make the signature of Chart.Column look like this:

image

Which of these signatures/API surfaces are more comprehensible for a C# user? This will influence hundreds of functions, so it is an important decision for Plotly.NET's C# API.

in both cases, you can call the function the same, so from a usage perspective these approaches are identical:

Chart.Column<int, string, string>(
    values: new int[] { 3, 4 },
    Keys: new string[] { "first", "second" },
    Width: 1,
    Base: 4
)

In-depth problem description

Plotly.NET's core F# API makes heavy use of generic optional parameters. Here is an example:

type Foo() =
    static member Bar(
        mandatory: string, 
        ?optNoProblemo1: int,
        ?optNoProblemo2: DateTime,
        ?optNoProblemo3: seq<#IConvertible>,
        ?optProblem: #IConvertible // this one is problematic
    ) =
        ...

for the respective C# Layer, we have to add type constraints on these optional parameters and make them nullable so we have a sure way of wrapping parameters that are set by the caller as Option.Some(value), and parameters not set by the caller as Option.None. This is a little awkwardness coming from optional parameter interop between the two, but that's not the problem:

public static int Bar<OptType>(
    string mandatory,
    int? optNoProblemo1 = null,
    DateTime? optNoProblemo2 = null,     
    IEnumerable<OptType>? optNoProblemo3 = null,
    OptType? optProblem = null
)
where OptType : IConvertible
=>
    Plotly.NET.Chart2D.Chart.Foo(
        mandatory: mandatory,
        optNoProblemo1: optNoProblemo1,
        optNoProblemo2: optNoProblemo2,
        optNoProblemo3: optNoProblemo3,
        optProblem: optProblem
    );

The problem is that optProblem cannot use null as default value, because it can be either reference or value type:

image

We also cannot use = default instead of = null, because then we have no way of checking if the value was actually set as a default value (in that case we want to convert to Some(value)) or if it was not set (and should therefore be wrapped as Option.None).

To fix this, we use a class Optional<T>:

public readonly record struct Optional<T>(T Value, bool IsValid)
    {
        public static implicit operator Optional<T>(T Value) => new(Value, true);
    }

which sets IsValid to true when a value is set, and false otherwise.

@kMutagene
Copy link
Collaborator Author

cc @JakeRadMSFT @luisquintanilla

@kMutagene
Copy link
Collaborator Author

kMutagene commented Jul 11, 2022

I personally prefer Option1, because then it is obvious that all optional parameters are handled the same. I am a F# guy though.

@ZimmerA
Copy link

ZimmerA commented Jul 11, 2022

As for which style to choose I would definitely go with option 1 because of consistency, which in my opinion is key when designing an API. Mixing things up like in option 2 can quickly lead to errors and confusion while option 1 will always work the same.

I ran into the same issues when developing CWLDotNet. I ended up using the OneOf library because I needed union types. Conveniently, it also allows OneOf<None, SomeType> which serves as an optional type.

https://github.com/louthy/language-ext#optional-and-alternative-value-monads also provides a more sophisticated optional type if you require some inspiration to improve yours.

@JakeRadMSFT
Copy link
Contributor

I’ll take a look on Monday! Sorry I just saw the tag.

@luisquintanilla
Copy link
Contributor

TLDR: Option 1.

Sorry for the delay. Thanks for the detailed description @kMutagene and discussion. I'm in favor of 1. Since usage is identical, I'd favor consistency which is what option 1 provides. In addition, when it comes to how concise the signature is, there's not a significant difference between both options.

@kMutagene
Copy link
Collaborator Author

Closing this for now, thanks for the feedback. The C# API will go with Option #1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants