Implement resource detection and generate ReourceData #47730
Conversation
github-actions
bot
added
Client
live1206
changed the title
live1206
requested review from
JoshLove-msft,
m-nash and
jorgerangel-msft
as code owners
live1206
requested review from
jsquire,
annelo-msft,
christothes,
KrzysztofCwalina,
tg-msft,
ArthurMa1978,
hallipr,
weshaggard and
benbp
as code owners
| { | ||
| if (inputType is InputModelType inputModel | ||
| && typeProvider is ModelProvider modelProvider | ||
| && AzureClientPlugin.Instance.OutputLibrary.IsResource(StringHelpers.ToCleanName(inputType.Name)) |
There was a problem hiding this comment.
Can we not use the modelProvider.Name?
There was a problem hiding this comment.
updated to use inputModel.Name directly
| // get the result from cache | ||
| if (_resourceDataSchemaCache.TryGetValue(set.RequestPath, out var resourceSchemaTuple)) | ||
| { | ||
| resourceSpecName = resourceSchemaTuple is null ? null : StringHelpers.ToCleanName(resourceSchemaTuple?.Name!); |
There was a problem hiding this comment.
Why do we need to call ToCleanName here?
There was a problem hiding this comment.
Yeah, given we always have InputModel while checking for resource, we can just keep the original InputModel name.
There was a problem hiding this comment.
Then, the only usage of ToCleanName is in the test, which I can live with using const string instead of it.
| } | ||
| } | ||
|
|
||
| return idPropertyFound && typePropertyFound && namePropertyFound; |
There was a problem hiding this comment.
Should we add a check for this after the switch within the foreach loop to short-circuit?
There was a problem hiding this comment.
Sounds good.
Added the check before the switch, if the switch case is hit it will skip the rest of the loop anyway.
| { | ||
| } | ||
|
|
||
| protected override string BuildName() => $"{base.BuildName()}Data"; |
There was a problem hiding this comment.
Could this be handled with a visitor instead?
There was a problem hiding this comment.
same reason as #47730 (comment)
|
|
||
| protected override string BuildName() => $"{base.BuildName()}Data"; | ||
|
|
||
| protected override string BuildRelativeFilePath() => Path.Combine("src", "Generated", $"{Name}.Serialization.cs"); |
There was a problem hiding this comment.
Does this need to be overriden?
There was a problem hiding this comment.
do you suggest using visitor to change the file location?
eng/packages/http-client-csharp/generator/Azure.Generator/src/AzureTypeFactory.cs
Outdated
Show resolved
Hide resolved
| if (inputType is InputModelType inputModel | ||
| && typeProvider is ModelProvider modelProvider | ||
| && AzureClientPlugin.Instance.OutputLibrary.IsResource(StringHelpers.ToCleanName(inputType.Name)) | ||
| && inputModel.Usage.HasFlag(InputModelTypeUsage.Json)) |
There was a problem hiding this comment.
Wondering if this logic can be moved into a visitor.
There was a problem hiding this comment.
Given the visitor is applied after all the type providers actually created.
If we want to change an existing type provider, we need to visit all the references they have been used, which is cumbersome.
There was a problem hiding this comment.
What I wrote above is concern about ModelProvider -> ResourceDataProvider.
For serialization, they might be fine since they are not referenced in other providers.
But then, do we want to treat them differently?
And should the sub-plugin have the ability to handle the serialization differently?
Resolves #47743