-
Notifications
You must be signed in to change notification settings - Fork 161
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
Add an ITypedValueExpressionFactory<T>
interface for our typed value expressions
#3223
base: main
Are you sure you want to change the base?
Add an ITypedValueExpressionFactory<T>
interface for our typed value expressions
#3223
Conversation
ITypedValueExpressionFactory<T>
interface for our typed value expressionsITypedValueExpressionFactory<T>
interface for our typed value expressions
@m-nash @ShivangiReja @jorgerangel-msft could you please have a review? This is ready to review now. |
API change check APIView has identified API level changes in this PR and created following API reviews. |
ITypedValueExpressionFactory<T>
interface for our typed value expressionsITypedValueExpressionFactory<T>
interface for our typed value expressions
@@ -36,5 +36,8 @@ public static BinaryDataExpression FromObjectAsJson(ValueExpression data) | |||
|
|||
public static BinaryDataExpression FromString(ValueExpression data) | |||
=> new(InvokeStatic(nameof(BinaryData.FromString), data)); | |||
|
|||
static BinaryDataExpression ITypedValueExpressionFactory<BinaryDataExpression>.Create(ValueExpression untyped) |
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.
Is there any reason we don't add explicit cast operators here instead?
Also should this fail if the ValueExpression given is not a BinaryDataExpression? How would we check that?
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 is supposed to wrap a untyped expression to this specific typed expression, works as the same as its ctor.
I use this to work as a constraint of "it has a ctor with parameter of valueexpression"
Also should this fail if the ValueExpression given is not a BinaryDataExpression? How would we check that?
No, our typed expressions do not work in this way.
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.
If this is the case "it has a ctor with parameter of valueexpression", why not just let the user call new BoolExpression(valueExpression) vs BoolExpression.Create(valueExpression). in dotnet land the new ctor syntax is much more discoverable?
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.
the whole point is not for how our use would construct this, despite this may have affects on that part.
This is trying to introduce a "generic constraint" so that we could have:
MethodBodyStatement Declare<T>(string name, T expression, out T variable)
as I have written in the description.
Adding this does not remove the ctor though. And I kind of do not want users (plugin writers) to call this method therefore I always explicitly implement those methods.
Fixes #3312
Majority of our typed value expressions will have the ctor taking a
ValueExpression
parameter, like this:and we cannot add a constraint using generic syntax, this cannot compile:
to have a generic method to catch all of the cases we could possibly support, we could use this newly introduced interface as the constraint:
where
ITypedValueExpressionFactory<T>
works similarly as the made-up constraint abovenew(ValueExpression)
.I believe this should simplify our code, and make it could automatically extend when a new expression is added.