Skip to content
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

Support @overload & @sharedRoute #1848

Merged
merged 27 commits into from
May 19, 2023
Merged

Support @overload & @sharedRoute #1848

merged 27 commits into from
May 19, 2023

Conversation

MaryGao
Copy link
Contributor

@MaryGao MaryGao commented May 17, 2023

fixes #1760 #1763

Leverage the TypeScript overload to support the decorators @SharedRoute and @overload.

Adding below test cases:

  • @SharedRoute for different positions including query, body, header etc
  • @overload for the same route, different routes and different actions
  • different media types supporting by union, overload and sharedRoute

@MaryGao MaryGao marked this pull request as ready for review May 17, 2023 03:27
@MaryGao MaryGao changed the title Support overload Support @overload & @sharedRoute May 17, 2023
id: string;
}
@route("/overload")
interface OveralodOperations {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joheredi Could you help review the overload cases and their generated code?

@@ -74,11 +75,15 @@ export function isDefinedStatusCode(statusCode: StatusCode) {
return statusCode !== "*";
}

export function isBinaryPayload(body: Type, contentType: string) {
export function isBinaryPayload(
Copy link
Member

Choose a reason for hiding this comment

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

Now that TypeSpec supports @encode I wonder if we can leverage it to get rid of this complex logic.

Copy link
Contributor Author

@MaryGao MaryGao May 18, 2023

Choose a reason for hiding this comment

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

@overload(OveralodOperations.getThing)
getString(@body param: string): string;

@overload(OveralodOperations.getThing)
Copy link
Member

Choose a reason for hiding this comment

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

@m-nash, did we get to an agreement on how we'll do overloads? Does this align with that?

Copy link
Member

Choose a reason for hiding this comment

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

I believe we agreed we would use sharedRoute in the main.tsp and remove the @overload decorator from azure.core and potentially add it for client.tsp only.

@srnagar can you confirm if that is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the current overload cases to client.tsp. I notice that we have an issue https://github.com/Azure/typespec-azure/issues/2738 in Project TOM but not started yet.

Copy link
Member

Choose a reason for hiding this comment

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

I believe we agreed we would use sharedRoute in the main.tsp and remove the @overload decorator from azure.core and potentially add it for client.tsp only.

@srnagar can you confirm if that is correct?

Yes, that is correct.

Copy link
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.

Do you mentioned that you will use the autorest testserver to write test cases for media types ?

@MaryGao
Copy link
Contributor Author

MaryGao commented May 18, 2023

Do you mentioned that you will use the autorest testserver to write test cases for media types ?

I re-organizated these test cases and their path can't be shared.

@MaryGao MaryGao merged commit f6799e3 into Azure:main May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for overload in RLC
5 participants