-
Notifications
You must be signed in to change notification settings - Fork 8
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
spike: demo the storage api in C# #554
Conversation
} | ||
|
||
// implicit cast from the various types into StoreValue | ||
public static implicit operator StoreValue(string value) => new StoreValue(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theoretically the right-hand-side of these could be a code block, correct? Allowing us to put in some logging or throw a different error type if we decide to, etc.?
Not saying we should change it from what you have here, just wanted to understand our options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The right-hand side here is just a shorthand when the function body is a single statement. We can have a multi-statement block instead.
await client.PutAsync("my-store", "my-key", "my-value"); | ||
await client.PutAsync("my-store", "my-key", 42); | ||
await client.PutAsync("my-store", "my-key", 3.14); | ||
await client.PutAsync("my-store", "my-key", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kewl
long value = success.Value; | ||
Console.WriteLine($"Success: {value}"); | ||
} | ||
catch (InvalidCastException) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one makes me want to raise the same question I raised on your JS PR about whether we should be modeling Success/Error as algebraic types, or just return the value directly and throw an exception for errors. In this case, that would simplify the code down to a try/catch block with multiple catch clauses, as opposed to an if+try.
Probably it'd be too inconsistent to make that dramatic of a change for this Client given that it will be living in the same SDK, but I'm just trying to think through what I would advocate for here if it was a greenfield product.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment re: the JS API. I like the shorthand but there's forward compatibility concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what sense? Are you talking about the case where the server changes in the future to support additional response types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind -- I was confusing the inner value type with the Success
object. You're saying we can return the Success
object or bust. We are free to add more data to Success
if we do that.
// This demos usage where the developer is sure of the type. | ||
{ | ||
var response = await client.GetAsync("my-store", "string-value"); | ||
if (response is StoreGet.Success success) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you seen this before?
https://spencerfarley.com/2021/03/26/unions-in-csharp/#improvements-in-c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It rings a bell. I think C# 9 wasn't out at the time when I saw this or it was a preview. When thinking about this I consider the lowest common denominator.
This demonstrates the Store API in C#. Because C# has significant support for implicit casts, we can greatly simplify the developer experience when interacting with the multitude of types that go in and out of the store.
By using implicit casts on the way in, we only need one
Put
method that takes aStoreValue
. TheStoreValue
type defines casts from various types toStoreValue
. That way a developer can write:and the runtime will create a
StoreValue
from the double. The compiler detects any invalid casts.Similarly on when we retrieve data with
GetAsync
, we can return aSuccess
that has aStoreValue
. The user can benefit from implicit casting in the reverse direction. They can writeIf the cast is incorrect, the .NET runtime will throw an
InvalidCastException
.By storing the type in
StoreValue
, we the developer can do pattern matching exhaustively over that. The compiler notifies the developer when the pattern matching is not exhaustive.