-
Notifications
You must be signed in to change notification settings - Fork 775
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
Refactor TypeProperty
to better represent ObjectType
state
#16053
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.
Copilot reviewed 48 out of 63 changed files in this pull request and generated no comments.
Files not reviewed (15)
- src/Bicep.Core.UnitTests/Rewriters/ReadOnlyPropertyRemovalRewriterTests.cs: Evaluated as low risk
- src/Bicep.Core.UnitTests/TypeSystem/DiscriminatedObjectTypeTests.cs: Evaluated as low risk
- src/Bicep.Core.UnitTests/TypeSystem/TypeValidatorAssignabilityTests.cs: Evaluated as low risk
- src/Bicep.Core.IntegrationTests/TypeSystem/TypeValidationTests.cs: Evaluated as low risk
- src/Bicep.Core.IntegrationTests/Scenarios/TopLevelResourcePropertiesTests.cs: Evaluated as low risk
- src/Bicep.Core.IntegrationTests/ScenarioTests.cs: Evaluated as low risk
- src/Bicep.Core.UnitTests/Utils/TestTypeHelper.cs: Evaluated as low risk
- src/Bicep.Core.UnitTests/TypeSystem/FunctionResolverTests.cs: Evaluated as low risk
- src/Bicep.Core.IntegrationTests/ScopeTests.cs: Evaluated as low risk
- src/Bicep.Core.UnitTests/Rewriters/TypeCasingFixerRewriterTests.cs: Evaluated as low risk
- src/Bicep.Core.IntegrationTests/ProviderImportTests.cs: Evaluated as low risk
- src/Bicep.Core.IntegrationTests/Extensibility/FooNamespaceType.cs: Evaluated as low risk
- src/Bicep.Core.UnitTests/TypeSystem/TypeHelperTests.cs: Evaluated as low risk
- src/Bicep.Core/Analyzers/Linter/Common/FindPossibleSecretsVisitor.cs: Evaluated as low risk
- src/Bicep.Core.UnitTests/Utils/BuiltInTestTypes.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)
src/Bicep.Core.UnitTests/Mock/FakeResourceTypes.cs:5
- The variable name 'resourcType' is misspelled. It should be 'resourceType'.
var resourcType = ResourceTypeReference.Parse(line);
Test this change out locally with the following install scripts (Action run 12997297695) VSCode
Azure CLI
|
Dotnet Test Results 76 files - 39 76 suites - 39 32m 59s ⏱️ - 19m 2s Results for commit b75451d. ± Comparison against base commit 5da4a85. This pull request removes 1840 and adds 636 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
Follow up to PR feedback.
With the current
ObjectType
signature, it's technically possible forAdditionalPropertiesDescription
to be non-null even ifAdditionalPropertiesType
is null. This PR creates a newNamedTypeProperty
record that represents a property with a name and a type, which is used for theObjectType.Properties
property.The
TypeProperty
record encapsulates the type ref, flags and description, preventing a non-null description with a null type ref being passed intoObjectType
.New
ObjectType
signature:public ObjectType(string name, TypeSymbolValidationFlags validationFlags, IEnumerable<NamedTypeProperty> properties, TypeProperty? additionalProperties, Func<ObjectType, FunctionResolver> methodResolverBuilder)
Microsoft Reviewers: Open in CodeFlow