-
Notifications
You must be signed in to change notification settings - Fork 319
Hierarchy building #8999
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?
Hierarchy building #8999
Conversation
|
No changes needing a change description found. |
...p/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs
Show resolved
Hide resolved
...ges/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs
Outdated
Show resolved
Hide resolved
...ges/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs
Show resolved
Hide resolved
...ges/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs
Show resolved
Hide resolved
...ges/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs
Outdated
Show resolved
Hide resolved
...ges/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs
Outdated
Show resolved
Hide resolved
...ges/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs
Outdated
Show resolved
Hide resolved
| public class ModelProvider : TypeProvider | ||
| { | ||
| private const string AdditionalBinaryDataPropsFieldDescription = "Keeps track of any properties unknown to the library."; | ||
| private const string DiscriminatorParameterName = "discriminatorValue"; |
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: I think we don't need these anymore
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 am still using it on line 823:
{ var discriminatorParam = new ParameterProvider( DiscriminatorParameterName, $"{DiscriminatorParameterDescription}", new CSharpType(typeof(string))); constructorParameters.Insert(0, discriminatorParam); }
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.
Once we adjust BuildConstructorParameters to have a parameter like includeHierarchyDiscriminator and pass that through to GetAllBaseProperties, we shouldn't need it anymore as it will be constructed automatically from the 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.
Woops we still need to remove these.
...ges/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs
Outdated
Show resolved
Hide resolved
...ges/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs
Outdated
Show resolved
Hide resolved
...ges/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs
Outdated
Show resolved
Hide resolved
|
Looks like the CI is failing |
b6bdf61 to
fa1ae2f
Compare
JoshLove-msft
left a comment
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.
Looks great!
This pull request introduces a new dual constructor pattern for intermediate models in discriminated union hierarchies, ensuring correct instantiation and inheritance when models share discriminator property names with their base models. It adds logic to detect when this pattern is needed, generates the appropriate constructors, and updates constructor initialization to support multi-layer discriminator scenarios. Several new tests are included to validate these changes.
Constructor Pattern Enhancements
ConstructorTypeenum to define different constructor generation strategies, supporting public, private protected, and internal constructors for various use cases.ModelProviderto detect when a model should use the dual constructor pattern (i.e., when it shares a discriminator property name with its base and has derived models).Constructor Initialization Logic
Testing Multi-Layer Discriminator Hierarchies
Minor Cleanup
discriminatedKindargument in a test setup for clarity.Addresses: Azure/azure-sdk-for-net#51958