-
Notifications
You must be signed in to change notification settings - Fork 75
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
Remove internal RLC layer from modular #2728
Conversation
3378852
to
9e8a859
Compare
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 think this is looking really good
9e8a859
to
62159e3
Compare
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.
thanks for taking care of this
returnType: `${rlcClientName}`, | ||
parameters: params, | ||
isExported: true | ||
const endpointParam = buildGetClientEndpointParam(factoryFunction, client); |
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.
feel like we might have missed the default logic handling for other parameters
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.
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 think this is covered here but let me know if there's something missing in that logic:
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.
the default value was handled in
Lines 30 to 32 in dd505de
const host = options.host ?? "one"; | |
const subdomain = options.subdomain ?? "two"; | |
const sufix = options.sufix ?? "three"; |
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 also think this kind of default value change can not be catched by the api view.md @joheredi this is a potential risk if we just check in the api view.md file.
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 don't think this is correct, we should respect the parameter default in the @server
decorators, that's why we should respect the endpoint default value, also in this case, those parameters are parts of the endpoint
Lines 30 to 36 in dd505de
const host = options.host ?? "one"; | |
const subdomain = options.subdomain ?? "two"; | |
const sufix = options.sufix ?? "three"; | |
const endpointUrl = | |
options.endpoint ?? | |
options.baseUrl ?? | |
`${host}.${subdomain}.${sufix}.com/${apiVersion}`; |
In this case, if we don't populating the clientDefaultValue, those parameters should be required and go into the getClient method signature instead of options.
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.
Can you give an example of what this example should look like?
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.
Is the issue here that apiVersion
isn't getting a default value when it should be? Or is there something else that is also not being considered?
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.
This is the typespec definition
autorest.typescript/packages/typespec-test/test/parametrizedHost/spec/main.tsp
Lines 20 to 32 in dd505de
@server( | |
"{host}.{subdomain}.{sufix}.com/{apiVersion}", | |
"Confidential Ledger Service", | |
{ | |
@path | |
host?: string = "one", | |
@path | |
subdomain?: string = "two", | |
@path | |
sufix?: string = "three", | |
@path | |
apiVersion?: string = "v1" | |
} |
@server
decorator.
if we choose to ignore the default value for it, then we should not handle the default value for endpoint or apiVersion either. the createParameterizedHost
function should be like
export function createParametrizedHost(
credential: TokenCredential,
host: string,
domain: string,
suffix: string,
apiVersion: string,
options: ParametrizedHostClientOptionalParams = {},
):
There're some more context here for how to handle endpoint and how to handle the clientDefaultValue
#2032 (comment)
#2049
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.
Is the issue here that
apiVersion
isn't getting a default value when it should be? Or is there something else that is also not being considered?
we have the default value for apiVersion https://github.com/Azure/autorest.typescript/pull/2561/files#diff-c381779e571bb42446df22900b2c16f1f7036a24f71042ac0bd83e4ef47ff630R28
if (options.initialRequestUrl || initialResponse) { | ||
response.headers["x-ms-original-url"] = | ||
options.initialRequestUrl ?? initialResponse!.request.url; | ||
} |
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.
why this is removed?
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.
The reason this header value was being set seems to be because the isUnexpected
helper was using it. Since we're not using isUnexpected
anymore I think it's ok to remove. Unless there's another place where we're using this value I don't know about?
...spec-ts/test/modularIntegration/generated/azure/core/lro/rpc/generated/src/api/operations.ts
Show resolved
Hide resolved
}; | ||
const clientContext = getClient(endpointUrl, credential, updatedOptions); | ||
clientContext.pipeline.removePolicy({ name: "ApiVersionPolicy" }); | ||
const apiVersion = options.apiVersion ?? "2023-11-15"; |
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.
@wanlwanl I thnk we need to change the version detection logic accordingly, after this pr get merged from src/rest/XXXClient.ts
to src/api/XXXContext.ts
This PR removes the
rest/
directory from the modular generator. There is no significant change in the public API and the runtime serialization logic remains the same.Key points:
PathUncheckedResponse
, and the deserialize functions accept aPathUncheckedResponse
instead of the RLC model types. The actual API methods still accept and return the API layer models.isUnexpected
, the status code of the response is now checked inline against a list of expected status codes.getClient
from@azure-rest/core-client
directly instead of creating an RLC client from the REST layer.getClient
used to do, such as adding the custom API version policy and/or key credential policy, are now done inline in the context factory function.Client
from@azure-rest/core-client
, instead of being a re-export of the RLC definition.After this PR, the actual emitted Modular code will not contain anything generated by the RLC emitter, excluding metadata the README and package.json. However, the emitter still relies on the RLC code model to construct the Modular code model. I plan to follow up on decoupling from the RLC code model and metadata generation shortly after this PR.
Fixes #2725.