-
Notifications
You must be signed in to change notification settings - Fork 325
Implement SystemObjectTypeProvider #7008
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
Conversation
|
No changes needing a change description found. |
| : MethodSignatureModifiers.Protected | MethodSignatureModifiers.Virtual; | ||
|
|
||
| if (_shouldOverrideMethods) | ||
| if (_shouldOverrideMethods && _model.BaseModelProvider is not SystemObjectTypeProvider) |
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.
While we generate Azure.ResourceManager with autorest.csharp, it only implements JsonModelWriteCore.
Once we generate it with MTG, we can remove this check for SystemObjectTypeProvider
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.
Please file an issue and reference it in a TODO comment.
|
|
||
| if (baseSystemProperties.TryGetValue(property.Name, out var baseSystemProperty)) | ||
| { | ||
| if (DomainEqual(baseSystemProperty, property, isBaseSystemProperty: true)) |
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.
BaseType from Azure.ResourceManager is not handling the property generation same as MTG, i.e. adding backing field and virtual for the derived property in BaseType.
So, we need to handle it differently.
Once we generate Azure.ResourceManager with MTG, we can remove this special handling.
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Security.Cryptography.X509Certificates; |
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.
?
| /// <summary> | ||
| /// Represent a type that already exists in dependencies | ||
| /// </summary> | ||
| public class SystemObjectTypeProvider : ModelProvider |
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.
How do we specify that this type already exists in the tsp?
| : MethodSignatureModifiers.Protected | MethodSignatureModifiers.Virtual; | ||
|
|
||
| if (_shouldOverrideMethods) | ||
| if (_shouldOverrideMethods && _model.BaseModelProvider is not SystemObjectTypeProvider) |
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.
Should this logic go into _shouldOverrideMethods?
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.
it can't, details is in #7008 (comment)
| var models = new List<TypeProvider>(input.Models.Count); | ||
| foreach (var inputModel in input.Models) | ||
| { | ||
| var outputModel = CodeModelGenerator.Instance.TypeFactory.CreateModel(inputModel); |
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 the model just be null if we shouldn't be generating it?
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 need the property and constructor signature from it while build the derived types.
If we return null here, we will need to use reflection to get the above information.
For current implementation, we don't use reflection like we did in autorest.csharp.
|
SystemObjectTypeProvider is only needed in MPG generator for now, implement it in MPG generator instead in Azure/azure-sdk-for-net#49653 |
Resolves #5480