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.

Sign up for GitHub

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

Jump to bottom

Fix issues in FACE AI spec #2484

Merged
merged 21 commits into from
Apr 29, 2024
Merged

Fix issues in FACE AI spec #2484

merged 21 commits into from
Apr 29, 2024

Conversation

MaryGao
Copy link
Member

@MaryGao MaryGao commented Apr 28, 2024

fixes #2474

@MaryGao MaryGao marked this pull request as ready for review April 28, 2024 10:43
@MaryGao MaryGao changed the title Fix the union issue in path parameter Fix issues in FACE AI spec Apr 28, 2024
MaryGao
MaryGao commented Apr 28, 2024
View reviewed changes
packages/autorest.typescript/test/rlcIntegration/generated/lroRest/src/isUnexpected.ts
"GET /lro/put/200/succeeded": ["200", "204"],
"PATCH /lro/patch/200/succeeded/ignoreheaders": ["200"],
"PUT /lro/put/200/succeeded": ["200", "204"],
Copy link
Member Author

Choose a reason for hiding this comment

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

just position change.

Copy link
Member

Choose a reason for hiding this comment

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

when are you going to fix this issue #2264 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is not relevant to the #2264 issue, the position changed because we adjust the isUnexcepected implementation a little bit.

qiaozha
qiaozha reviewed Apr 29, 2024
View reviewed changes
packages/autorest.typescript/test/rlcIntegration/generated/lroRest/src/isUnexpected.ts
"GET /lro/put/200/succeeded": ["200", "204"],
"PATCH /lro/patch/200/succeeded/ignoreheaders": ["200"],
"PUT /lro/put/200/succeeded": ["200", "204"],
Copy link
Member

Choose a reason for hiding this comment

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

when are you going to fix this issue #2264 ?

packages/typespec-test/test/eventgrid_modular/generated/typespec-ts/src/rest/isUnexpected.ts
Comment on lines -31 to +32
response:
| PublishCloudEvent200Response
| PublishCloudEvents200Response
| PublishCloudEventDefaultResponse,
response: PublishCloudEvent200Response | PublishCloudEventDefaultResponse,
Copy link
Member

Choose a reason for hiding this comment

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

what is causing the changes here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

bug here, see comments #2484 (comment)

packages/typespec-ts/src/transform/transformPaths.ts Outdated Show resolved Hide resolved
packages/typespec-ts/test/integration/generated/sharedRoute/src/clientDefinitions.ts
@@ -59,7 +65,7 @@ export interface ProcessInt {

export interface Routes {
/** Resource for '/sharedroute/query' has methods for the following verbs: post */
(path: "/sharedroute/query"): ListByResourceGroup;
(path: "/sharedroute/query"): ListBySubscription;
Copy link
Member

Choose a reason for hiding this comment

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

what is causing the change here ? operation order change?

Copy link
Member Author

@MaryGao MaryGao Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

no the bug is introduced by overload. previously we combined the overloading responses together and this would introduce issues for non-matching logical response.

packages/typespec-ts/test/integration/generated/sharedRoute/src/isUnexpected.ts
@@ -0,0 +1,114 @@
// Copyright (c) Microsoft Corporation.
Copy link
Member

Choose a reason for hiding this comment

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

why this is not generated before?

Copy link
Member Author

Choose a reason for hiding this comment

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

i update the tsp, previously there is only one status code defined so no isUnexpected helper would generate.

packages/typespec-ts/test/integration/generated/sharedRoute/src/responses.ts Outdated

/** There is no content to send for this request, but the headers may be useful. */
export interface ListBySubscription204Response extends HttpResponse {
status: "204";
Copy link
Member

Choose a reason for hiding this comment

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

why is the response code 204 for list operation? and is it really expected to have no body in list response ?

Copy link
Member Author

Choose a reason for hiding this comment

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

okay i just created an test case for different status code would return different body shape.

I change the code to 202 now.

packages/typespec-ts/test/unit/clientDefinitionGenerator.spec.ts
@@ -68,4 +71,129 @@ describe("Client definition generation", () => {
`
);
});

describe("union & enum parameter in path", () => {
Copy link
Member

Choose a reason for hiding this comment

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

can you add test cases for union and enum being used in query and header ?

Copy link
Member Author

@MaryGao MaryGao Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Added the cases for query. And there exist cases in header: https://github.com/Azure/autorest.typescript/blob/main/packages/typespec-ts/test/unit/modelsGenerator.spec.ts#L3324-L3767

packages/rlc-common/src/interfaces.ts Outdated
@@ -135,7 +136,8 @@ export interface PagingDetails {
}

export type Methods = {
[key: string]: [OperationMethod];
// could be more than one method if overloading
[key: string]: [OperationMethod, ...OperationMethod[]];
Copy link
Member

Choose a reason for hiding this comment

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

Is it trying to suggest there's at least one OperationMethod ? is it necessary?

Copy link
Member Author

@MaryGao MaryGao Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

change to OperationMethod[]

packages/typespec-test/test/faceai/generated/typespec-ts/src/isUnexpected.ts Show resolved Hide resolved
qiaozha
qiaozha approved these changes Apr 29, 2024
View reviewed changes
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.

LGTM

@MaryGao MaryGao merged commit 09685d1 into Azure:main Apr 29, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiaozha qiaozha qiaozha approved these changes

@sarangan12 sarangan12 Awaiting requested review from sarangan12 sarangan12 is a code owner

@joheredi joheredi Awaiting requested review from joheredi joheredi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix the ci failures in FaceAI
3 participants
@MaryGao @qiaozha @v-jiaodi