-
Notifications
You must be signed in to change notification settings - Fork 76
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
Fix sample generation for several mgmt plane RPs #1289
Fix sample generation for several mgmt plane RPs #1289
Conversation
Can you please add a description about what's the problem and how are you fixing it? It is very difficult to make sense of a PR without proper description |
@joheredi I have updated the description for this PR. Could you take a look again ? |
test/smoke/generated/network-resource-manager/samples-dev/virtualWanCreate.ts
Show resolved
Hide resolved
...e/generated/network-resource-manager/samples-dev/startPacketCaptureOnVpnGatewayWithFilter.ts
Outdated
Show resolved
Hide resolved
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'll defer the approval to @sarangan12, from my side, I'd like the unnecessary cast to be removed.
test/smoke/generated/storage-resource-manager/samples-dev/acquireALeaseOnAShare.ts
Outdated
Show resolved
Hide resolved
I think all the comments from @joheredi should have been resolved. Feel free to comment or correct me if it's not. Thanks |
@@ -8,8 +8,10 @@ | |||
* x-ms-original-file: {{originalFileLocation}} | |||
*/ | |||
import { | |||
{{#if hasBody }} | |||
{{bodySchemaName}}, |
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.
If we don't use these two columns does that mean we could delete them in model and transform.ts?
@@ -15,4 +15,6 @@ export interface SampleDetails { | |||
isTopLevel: boolean, | |||
isPaging: boolean, | |||
originalFileLocation?: string | |||
isAnyTypeBody?: boolean, |
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 didn't notice any usage for this column - isAnyTypeBody, does that mean we could delete it?
@@ -18,6 +18,8 @@ import { calculateMethodName } from "../generators/utils/operationsUtils"; | |||
import { camelCase } from "@azure-tools/codegen"; | |||
import { OperationGroupDetails } from "../models/operationDetails"; | |||
import { getPublicMethodName } from '../generators/utils/pagingOperations'; | |||
import { BodiedNode } from "ts-morph"; |
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.
No usage delete this import?
sample.importedTypes?.push(parameterTypeName); | ||
if (methodParameter.exampleValue.schema.type === SchemaType.AnyObject || methodParameter.exampleValue.schema.type === SchemaType.Any) { | ||
sample.bodySchemaName = "Record<string, unknown>" | ||
sample.isAnyTypeBody = 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.
We re-correct the value of bodySchemaName & isAnyTypeBody but we didn't put the latest into importedTypes list or any usage directly, do we need to refine this?
This PR is trying to fix several issues for mgmt plane RPs.