URI template support in RLC #2814
Conversation
...pespec-test/test/translator/generated/typespec-ts/review/cognitiveservices-translator.api.md
Outdated
Show resolved
Hide resolved
packages/typespec-test/test/faceai/generated/typespec-ts/src/serializeHelper.ts
Outdated
Show resolved
Hide resolved
| @@ -312,7 +313,8 @@ function generateRLCIndex(file: SourceFile, model: RLCModel) { | |||
| hasSsvCollection(model) || | |||
| hasPipeCollection(model) || | |||
| hasTsvCollection(model) || | |||
| hasCsvCollection(model) | |||
| hasCsvCollection(model) || | |||
| (model.helperDetails?.parameterBuilders ?? []).length > 0 | |||
There was a problem hiding this comment.
should we remove the code for ssv and other not going to support collection helpers codegen logic ?
There was a problem hiding this comment.
i plan to remove all logic in modular pr, currently modular still have this dependency.
| @@ -130,7 +172,7 @@ export declare interface QueryTsvQueryParam { | |||
| } | |||
|
|
|||
| export declare interface QueryTsvQueryParamProperties { | |||
| colors: string; | |||
| colors: string[]; | |||
There was a problem hiding this comment.
is this expected to get this in the tsv case even the compatibilityMode is enabled?
There was a problem hiding this comment.
yes, we only keep original code for multi cases and which only happened in reality.
| export declare interface QueryParametersQueryContinuationStandardRecordQueryParam { | ||
| queryParameters: QueryParametersQueryContinuationStandardRecordQueryParamProperties; | ||
| } | ||
|
|
||
| export declare interface QueryParametersQueryContinuationStandardRecordQueryParamProperties { | ||
| param: Record<string, number>; | ||
| param: QueryParametersQueryContinuationStandardRecordParamQueryParam; |
There was a problem hiding this comment.
is there a case where value type is array of objects?
There was a problem hiding this comment.
for complicated nested cases e.g objected nested array or array nested objects i don't think we have agreement on how to serialize them correctly. our core is not ready either.
i think this would be a cross-language issue and prefer to defer any discussion for them for now considering no real usage.
| assert.deepEqual(result.body, responseBody); | ||
| }); | ||
|
|
There was a problem hiding this comment.
why remove the catch error part in this case? what about other cases?
There was a problem hiding this comment.
removed useless codes here. we don't need to catch any error to fail. if exception happens the case would fail directly.
i updated cases i met in this pr. but i remember i told jiaodi to remove for any new cases.
| export declare type QueryPipesParameters = QueryPipesQueryParam & RequestParameters; | ||
|
|
||
| export declare interface QueryPipesQueryParam { | ||
| queryParameters: QueryPipesQueryParamProperties; | ||
| } | ||
|
|
||
| export declare interface QueryPipesQueryParamProperties { | ||
| colors: string; | ||
| colors: QueryPipesColorsQueryParam; |
There was a problem hiding this comment.
why this is not show as compatibleMode?
There was a problem hiding this comment.
changed the option to compatibilityQueryMultiFormat.
| const operationFiles = await emitModularOperationsFromTypeSpec( | ||
| tspContent, | ||
| { | ||
| mustEmptyDiagnostic: false |
There was a problem hiding this comment.
what diagnostic warning will we get? is it the query format deprecation warning? but why it's not reported before query format should be deprecated a long time ago ?
There was a problem hiding this comment.
this is a bugfix and here we should report diagnostic of unable-serialized-type for header but not: https://github.com/marygao/autorest.typescript/blob/uri-template-support/packages/typespec-ts/src/utils/parameterUtils.ts#L152-L156.
so the new warning is introduced by this pr's fix.
| const clientDef = await emitParameterFromTypeSpec( | ||
| ` | ||
| @route("template/{+param}") | ||
| op template(param: string): void; |
There was a problem hiding this comment.
can you add tests to test against the generated Routes and generated helpers?
There was a problem hiding this comment.
updated the Routes generation.
| importInfo: { | ||
| internalImports: importSet, | ||
| runtimeImports: buildRuntimeImports("azure") | ||
| }, | ||
| options: { | ||
| sourceFrom: "TypeSpec" |
There was a problem hiding this comment.
if we didn't set this this would be undefined. i just set it explicitly in code.
in our code we would set it as TypeSpec.
| @@ -153,6 +153,16 @@ If we enable this option `clearOutputFolder` we would empty the whole output fol | |||
| clearOutputFolder: true | |||
| ``` | |||
|
|
|||
| ### compatibilityMode | |||
There was a problem hiding this comment.
can you use a new option, in the url template case, compatibilityMode is not fully backward compatible, you still drop the support for ssv and tsv, pipe etc. While in additionaly properties case, it is really backward compatible.
There was a problem hiding this comment.
updated as compatibilityQueryMultiFormat option.
packages/typespec-ts/test/integration/generated/parameters/collection-format/src/index.d.ts
Outdated
Show resolved
Hide resolved
MaryGao
force-pushed
the
uri-template-support
branch
from
21fe31c
to
9504be9
Compare
fixes #2793
fixes #2792
Breakings
CollectionFormat with csv and multi are most commonly used cases, for
csvthere is no breaking. And formultithere will be a breaking and enabling the optioncompatibilityQueryMultiFormat: truewould mitigate this breaking.CollectionFormat with tsv is no longer supported and we are not aware of any real cases yet.
Possible values
In-scoped cases
Path allowReserved
Support to define allowReserved for path parameter but the style and explode support for path parameter is not in-scope of this pr. The default for allowReserved would be
Query expansion
Please note the allowReserved for query paremeter is not in scope of this pr. Except the form style we would also include the spaceDelimited and pipeDelimited mainly considering backward-compatibility. The default setting would be:
/users{?id}/users?id=5/users?id=3,4,5/users?id=role,admin,firstName,Alex/users{?id*}/users?id=5/users?id=3&id=4&id=5/users?role=admin&firstName=Alex/users?id=3|4|5Out-of-scope cases