New NRP Apiversion, vnet peering, Get Effective route/acls, swagger improvements #379
Conversation
|
Hi @DeepakRajendranMsft, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution! TTYL, AZPRBOT; |
| "x-ms-pageable": { | ||
| "nextLinkName": "nextLink" | ||
| }, | ||
| "x-ms-long-running-operation": true |
|
@John-Hart fixed indentation |
| "properties": { | ||
| "networkSecurityGroup": { | ||
| "$ref": "#/definitions/SubResource", | ||
| "description": "Gets the id of network security group that is applied" |
There was a problem hiding this comment.
Autorest will add "Gets or sets" as a prefix to the description for properties, for languages where appropriate, or just "Gets" for ReadOnly properties for you, so there is no need to specify them here.
In fact, in this example you can see the description ends up not being very readable:
"Gets or sets gets the id of network security group that is applied".
Since this isn't identified as ReadOnly it will append "Gets or sets"
There was a problem hiding this comment.
Also sibiling properties of the $ref will be ignored. The Description, should be in the reference definition
|
@John-Hart , we have been using "Gets or sets" everywhere in network swagger. We were not aware when AutoRest started adding "Gets or sets" automatically. We should deal with this and remove them as part of a separate PR. |
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "", |
There was a problem hiding this comment.
Could you have a more descriptive description here?
|
@DeepakRajendranMsft regarding the Gets or sets in the descriptions. That's fine if you want to remove them all in another PR. In the mean time, I've submitted an autorest PR to not prefix the description if it already contains Gets or Sets so it's still readable |
|
@DeepakRajendranMsft though I do agree with you that we should have separate, small PRs, removing the "Get and Sets" is just a simple find and replace. Do you think we can expand this PR just a little bit and get this cleaned up? |
|
@DeepakRajendranMsft Could you check if the new file has a valid paging usage? To avoid this: #349 |
|
@devigned @John-Hart we have already generated the SDKs and are using them in powershell. I will definitely fix them, but not immediately. Since the SDK is already published, we should check in the swagger asap so we can avoid getting out of sync. This is why i asking for you to track your feedback in either bugs or github issues and i will address them ASAP. |
|
Merged |
|
I opened two issues #386 #385 for the blank descriptions and the duplicate Gets and Sets in the descriptions. I didn't open one for the Default responses because, even though you have an #/definitions/ErrorDetails and #/definitions/Error defined it doesn't look like they are being used. If they are then defining a Default Response would be required |
|
No modification for Python |
|
No modification for NodeJS |
|
No modification for Python |
|
No modification for NodeJS |
No description provided.