-
Notifications
You must be signed in to change notification settings - Fork 36
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
Fix code generation for Durable Functions function-based syntax. #47
Conversation
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.
LGTM!
@cgillum I found another case to fully qualify the type name. |
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.
I took a slightly deeper look this time through and have a few more questions that didn't come up from earlier iterations.
{ | ||
this.FullTypeName = fullTypeName; | ||
this.RequiredNamespaces = requiredNamespaces; | ||
this.Name = name; | ||
this.Kind = kind; | ||
this.Parameter = parameter; | ||
this.ReturnType = returnTypeSyntax; | ||
this.ReturnType = SyntaxNodeUtility.GetRenderedTypeExpression(returnType, false); |
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.
Does this false
mean we don't support nullable annotations in return types?
return false; | ||
} | ||
|
||
if (!SyntaxNodeUtility.TryGetQualifiedTypeName(model, method, out string? fullTypeName)) | ||
{ | ||
function = null; | ||
return false; | ||
} |
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.
Would it make sense to move the TryGetParameter
and TryGetQualifiedName
checks further up, above the new return type code you added? Could that short-circuiting help save some CPU cycles or do we very rarely expect those methods to return false?
{ | ||
return false; | ||
} | ||
|
||
if (toProcess is not PredefinedTypeSyntax && typeInfo.Type.ContainingNamespace.IsGlobalNamespace) | ||
if (typeInfo.ContainingNamespace.IsGlobalNamespace) | ||
{ | ||
requiredNamespaces = null; | ||
return false; |
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.
I know you didn't write this code, but is it correct for us to bail like this if we encounter a global namespace? Why wouldn't we continue to process remaining types in the remaining
list?
string expression = symbol.ToString(); | ||
if (expression.StartsWith("System.") && symbol.ContainingNamespace.Name == "System") | ||
{ | ||
expression = expression.Substring("System.".Length); |
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.
We do this because the System
namespace is implicitly imported by the compiler?
I have opened #48 to track the comments here and other improvements to the generator. |
This PR addresses 2 issues with function based syntax code gen for Durable Functions:
Task<T>
would get double nested asTask<Task<T>>
resolves #35