Skip to content

Fully support url template in Modular#2884

Merged
qiaozha merged 83 commits intoAzure:mainfrom
MaryGao:poc-modular-uri-template-support
Mar 7, 2025
Merged

Fully support url template in Modular#2884
qiaozha merged 83 commits intoAzure:mainfrom
MaryGao:poc-modular-uri-template-support

Conversation

@MaryGao
Copy link
Copy Markdown
Member

@MaryGao MaryGao commented Oct 28, 2024

fixes #2735
fixes #2794
fixes #2866
fixes #3000

The pr is quite direct and I implemented an url template resolution helper to resolve path and query parameters. Relevant UTs and integration tests are updated to cover the new feature.

Comment thread packages/typespec-ts/test/modularIntegration/azureCore.spec.ts
Comment thread packages/typespec-ts/static/static-helpers/uriTemplate.ts Outdated
Comment thread packages/typespec-ts/test/azureModularIntegration/routes.spec.ts Outdated
Comment thread packages/typespec-ts/static/static-helpers/urlTemplate.ts
Comment thread packages/typespec-ts/static/static-helpers/urlTemplate.ts Outdated
@qiaozha qiaozha requested review from Copilot and removed request for sarangan12 February 19, 2025 07:59
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 68 out of 82 changed files in this pull request and generated no comments.

Files not reviewed (14)
  • packages/typespec-ts/package.json: Language not supported
  • packages/typespec-ts/test/azureModularIntegration/routes.spec.ts: Evaluated as low risk
  • packages/typespec-ts/test/modularUnit/scenarios/operations/pathParam/allowReservedTrueWithUriTemplate.md: Evaluated as low risk
  • packages/typespec-ts/src/utils/operationUtil.ts: Evaluated as low risk
  • packages/typespec-ts/src/index.ts: Evaluated as low risk
  • packages/typespec-ts/test/modularIntegration/collectionFormat.spec.ts: Evaluated as low risk
  • packages/typespec-ts/test/commands/cadl-ranch-list.js: Evaluated as low risk
  • packages/typespec-ts/src/modular/static-helpers-metadata.ts: Evaluated as low risk
  • packages/typespec-ts/src/modular/buildOperations.ts: Evaluated as low risk
  • packages/typespec-ts/test/modularUnit/scenarios/example/example.md: Evaluated as low risk
  • packages/typespec-ts/test/modularUnit/scenarios/operations/pathParam/allowReservedWithUriTemplate.md: Evaluated as low risk
  • packages/typespec-ts/test/modularUnit/scenarios/operations/pathParam/allowReservedFalseInAnnotation.md: Evaluated as low risk
  • packages/typespec-ts/test/modularUnit/scenarios/anonymous/anonymous.md: Evaluated as low risk
  • packages/typespec-ts/test/modularUnit/scenarios/apiOperations/apiOperations.md: Evaluated as low risk
Comments suppressed due to low confidence (1)

packages/typespec-ts/src/modular/helpers/operationHelpers.ts:1059

  • The removal of the allowReserved check for path parameters in the getPathParamExpr function could lead to incorrect URL encoding when reserved characters are involved. Please reintroduce the allowReserved check for path parameters.
const value = defaultValue ? typeof defaultValue === "string" ? `${paramName} ?? "${defaultValue}"` : `${paramName} ?? ${defaultValue}` : paramName;

Comment thread packages/typespec-ts/src/modular/buildOperations.ts Outdated
Comment thread packages/typespec-ts/static/static-helpers/urlTemplate.ts
Comment thread packages/typespec-ts/test/azureModularIntegration/routes.spec.ts Outdated
Comment thread packages/typespec-ts/package.json Outdated
Copy link
Copy Markdown
Member

@timovv timovv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Codegen changes look good, and I see we have good test coverage for the implementation of the static helper. I think this is good as a first cut

Comment thread packages/typespec-ts/static/static-helpers/urlTemplate.ts
Comment thread packages/typespec-ts/test/modularUnit/static/urlTemplate.spec.ts
Comment thread packages/typespec-ts/test/modularUnit/scenarios/example/example.md
Comment thread packages/typespec-ts/test/modularUnit/static/urlTemplate.spec.ts
Copy link
Copy Markdown
Member

@qiaozha qiaozha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create issues for headers url template support and the deprecation of skipUrlEncoding in core, let's have further communications there.

@qiaozha qiaozha merged commit d5c5977 into Azure:main Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

7 participants