refactor(http-client-csharp): dump deprecated description properties
#5154
Conversation
- emitter: replace all `description` properties of input types with `summary` and `doc` - generator: - update json converters for the emitter schema change - only expose `Summary` and `Doc` properties if they are used by other classes - update test cases accordingly part of microsoft#4771
microsoft-github-policy-service
bot
added
the
emitter:client:csharp
archerzz
changed the title
filter-out-core-models optiondescription properties
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
| private static readonly InputClient _animalClient = new("animal", "", "AnimalClient description", [], [], TestClientName); | ||
| private static readonly InputClient _dogClient = new("dog", "", "DogClient description", [], [], _animalClient.Name); | ||
| private static readonly InputClient _catClient = new("cat", "", "CatClient description", [], [], _animalClient.Name); | ||
| private static readonly InputClient _hawkClient = new("hawkClient", "", "HawkClient description", [], [], _animalClient.Name); | ||
| private static readonly InputClient _huskyClient = new("husky", "", "HuskyClient description", [], [], _dogClient.Name); |
There was a problem hiding this comment.
i think we should do string.Empty instead of "".
Also should this be null?
There was a problem hiding this comment.
I think it is never required to have summary or doc there these two properties should be nullable.
And there should be difference between when we have:
model Foo {}
and
@doc("")
model Foo {}
There was a problem hiding this comment.
i think we should do
string.Emptyinstead of"". Also should this benull?
It cannot be null, because nullable check is enabled.
There was a problem hiding this comment.
I think it is never required to have summary or doc there these two properties should be nullable. And there should be difference between when we have:
model Foo {}and
@doc("") model Foo {}
Zero values are the same here. You don't want to generate an empty doc anyway. Also, we're not consistent in our code base. Some defines string? and others define string. I just follow what the current codes were written.
There was a problem hiding this comment.
This comment follows this one: #5154 (comment)
For example an input enum type is not required to have a doc or summary or description, therefore our definition here is wrong in the first place.
We define this in this way, because previously these are defined for azure purpose, and for azure libraries, types, properties are not allowed without documentation. But in unbranded libraries, there is no such rules.
Azure part should change accordingly if we change it here - assign default value when an output type is constructed.
There was a problem hiding this comment.
changed all Summary and Doc as string?
| : base(name) | ||
| { | ||
| CrossLanguageDefinitionId = crossLanguageDefinitionId; | ||
| Accessibility = accessibility; | ||
| Deprecated = deprecated; | ||
| Description = description; | ||
| Description = string.IsNullOrEmpty(summary) ? (doc ?? string.Empty) : summary; |
There was a problem hiding this comment.
I think we should change this property to two new properties Summary and Doc
The classes in input types should just be the C# version of those typescript interfaces.
There was a problem hiding this comment.
I see two different strategies regarding the input types between http-client-csharp and autorest.csharp:
- In
http-client-csharp, we declare them asclass. So, I keep the minimal interface here. - In
autorest.csharp, we declare them asrecord. So, I keep the interface like the original data schema as much as possible.
There was a problem hiding this comment.
record is a class, there is no differences.
Making this change might take quite a few efforts, this I admit.
But we should not be avoiding them - I have done multiple of those when consolidating the models part.
There was a problem hiding this comment.
removed all Description properties, replace with codes in the output model
|
Changes since last review:
|
| @@ -424,7 +425,7 @@ private ScmMethodProvider BuildProtocolMethod(MethodProvider createRequestMethod | |||
|
|
|||
| var methodSignature = new MethodSignature( | |||
| isAsync ? _cleanOperationName + "Async" : _cleanOperationName, | |||
| FormattableStringHelpers.FromString(Operation.Description), | |||
| DocHelpers.GetFormattableDescription(Operation.Summary, Operation.Doc) ?? FormattableStringHelpers.FromString(Operation.Name), | |||
There was a problem hiding this comment.
Description of MethodSignature is nullable. Why do we mock the operation.Name as its description here?
There was a problem hiding this comment.
Description of MethodSignature is nullable. Why do we mock the operation.Name as its description here?
It's because previously we use Operation.Name if Description is not available:
After discussion with Dapeng, we thought we should do less in the input type converters. So, I moved the logic in TypeSepcInputOperationConverter to the output model here.
We have similar changes elsewhere in this PR as well.
descriptionproperties of input types withsummaryanddocSummaryandDocproperties if they are used by other classespart of #4771