-
Notifications
You must be signed in to change notification settings - Fork 757
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 support for name discriminators on resource types #1950
Conversation
@@ -70,6 +70,7 @@ public static class LanguageConstants | |||
public const string ResourceScopePropertyName = "scope"; | |||
public const string ResourceParentPropertyName = "parent"; | |||
public const string ResourceDependsOnPropertyName = "dependsOn"; | |||
public const string TypeNameString = "string"; |
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.
TypeNameString [](start = 28, length = 14)
Would StringTypeName
be better?
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 thought about this - I'm going less specific -> more specific left-to-right so that e.g. TypeNameInteger
, TypeNameString
, TypeNameWhateverElse
are grouped alphabetically.
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.
Which is different from most of the constants in the class :P
In reply to: 598960954 [](ancestors = 598960954)
@@ -11,11 +11,11 @@ public class DiscriminatedObjectType : TypeSymbol | |||
public DiscriminatedObjectType(string name, TypeSymbolValidationFlags validationFlags, string discriminatorKey, IEnumerable<ITypeReference> unionMembers) | |||
: base(name) | |||
{ | |||
var unionMembersByKey = new Dictionary<string, ITypeReference>(); | |||
var unionMembersByKey = new Dictionary<string, NamedObjectType>(); |
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.
NamedObjectType [](start = 59, length = 15)
Wouldn't this block nested discriminators?
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, but if you take a look at line 18 - you'll see we're already blocking it. I'd suggest opening an issue if you think it's something we should support.
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.
name: '${db.name}/current' // had to change databaseName => db.name to get dependsOn working | ||
resource tde 'Microsoft.Sql/servers/databases/transparentDataEncryption@2020-08-01-preview' = { | ||
parent: db | ||
name: 'current' // had to change databaseName => db.name to get dependsOn working |
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: the comment is no longer relevant
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.
Good point - created #2166
WIP - requires Azure/bicep-types-az#153, and tests
Fixes #657