-
Notifications
You must be signed in to change notification settings - Fork 814
Add extension namespace functions #18910
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
base: main
Are you sure you want to change the base?
Conversation
|
Test this change out locally with the following install scripts (Action run 21645183295) VSCode
Azure CLI
|
| ("main.bicep", new(""" | ||
| extension '../extension.tgz' as ext | ||
| var foo = ext.config('redis') | ||
| """)), |
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.
[Nit]: With """ blocks, multiline snippets don't have to be flush with the left margin. I try to keep them bounded by one indentation level past the line they appear in, but opinions on what's most readable vary.
| // Ideally extension namespace function authors shouldn't need to set this flag in the types.json at all if they have externalInputs in the "evaluatesTo" property | ||
| // Consider using visitor pattern to determine this automatically | ||
| if (!functionSymbol.FunctionFlags.HasFlag(FunctionFlags.RequiresExternalInput)) | ||
| catch (Exception) |
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 way to make this more specific? What exceptions are expected here?
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.
Added handling and unit test for ExpressionException if there is a failure running the arm expression evaluator.
| if (intermediate is not FunctionCallExpression functionExpression || functionExpression.Parameters.Length < 1) | ||
| private void CollectExternalInputs(FunctionCallSyntaxBase sourceSyntax, FunctionExpression functionExpression) | ||
| { | ||
| if (!functionExpression.NameEquals(LanguageConstants.ExternalInputBicepFunctionName)) |
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 NameEquals case insensitive?
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, it's case-insensitive.
bicep/src/Bicep.Core/Extensions/LanguageExpressionExtensions.cs
Lines 9 to 10 in c378ace
| public static bool NameEquals(this FunctionExpression expression, string name) | |
| => StringComparer.OrdinalIgnoreCase.Equals(expression.Function, 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.
Could you add a couple of tests here? I think there should be tests covering:
- The result of one namespace function being passed as an argument to another.
- A namespace function where
externalInputis part of a function property
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.
Added
c378ace to
4f6e5df
Compare
Description
LanguageExpression. This is because the extension namespace function may define the expression that it should be reduced to which may not necessarily be the same as the function syntax.Example Usage
Namespace function definition:
Bicepparam:
Compiled json:
Checklist
Microsoft Reviewers: Open in CodeFlow