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

cadl-ranch-specs, add basic test case of clientInitialization #727

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

weidongxu-microsoft
Copy link
Member

Cadl Ranch Contribution Checklist:

  • I have written a scenario spec
  • I have meaningful @scenario names. Someone can look at the list of scenarios and understand what I'm covering.
  • I have written a mock API
  • I have used @scenarioDocs for extra scenario description and to tell people how to pass my mock api check.

Copy link

changeset-bot bot commented Sep 12, 2024

🦋 Changeset detected

Latest commit: 7fb502f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@azure-tools/cadl-ranch-specs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@chunyu3 chunyu3 left a comment

Choose a reason for hiding this comment

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

We may need add more complex cases:

  • more complex client hierarchy, e.g. more than 3 layer clients
  • co-operate with @client / @operationGroup decorator
  • with paramMaps to map a parameter to the property of the Client initialization Model
  • a senario that not all operations has the bumped parameter

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Sep 12, 2024

We may need add more complex cases:

  • more complex client hierarchy, e.g. more than 3 layer clients
  • co-operate with @client / @operationGroup decorator
  • with paramMaps to map a parameter to the property of the Client initialization Model
  • a senario that not all operations has the bumped parameter

Wondering whether we need to have a subfolder for different scenarios. And this be in a "basic" folder.

As the characteristic of the "client hierarchy", some of them cannot co-exist in same main.tsp. E.g. client by namespace/interface cannot be in same file as client by @client/@operationGroup.

@chunyu3
Copy link
Member

chunyu3 commented Sep 12, 2024

We may need add more complex cases:

  • more complex client hierarchy, e.g. more than 3 layer clients
  • co-operate with @client / @operationGroup decorator
  • with paramMaps to map a parameter to the property of the Client initialization Model
  • a senario that not all operations has the bumped parameter

Wondering whether we need to have a subfolder for different scenarios. And this be in a "basic" folder.

As the characteristic of the "client hierarchy", some of them cannot co-exist in same main.tsp. E.g. client by namespace/interface cannot be in same file as client by @client/@operationGroup.

Yes. Those are separate scenarios and may not co-exist and should be placed in different folder.

github-merge-queue bot pushed a commit to microsoft/typespec that referenced this pull request Sep 12, 2024
This is only part of the support for
Azure/autorest.java#2428

cadl-ranch Azure/cadl-ranch#727

user code
```java
        InitializationClient client = new InitializationClientBuilder()
            .name("client1")
            .buildClient();
        client.action();
```
@weidongxu-microsoft weidongxu-microsoft marked this pull request as ready for review September 12, 2024 08:22
@weidongxu-microsoft weidongxu-microsoft changed the title specs, add test case of clientInitialization specs, add basic test case of clientInitialization Sep 12, 2024
sarangan12 pushed a commit to sarangan12/typespec that referenced this pull request Sep 16, 2024
This is only part of the support for
Azure/autorest.java#2428

cadl-ranch Azure/cadl-ranch#727

user code
```java
        InitializationClient client = new InitializationClientBuilder()
            .name("client1")
            .buildClient();
        client.action();
```
@weidongxu-microsoft weidongxu-microsoft changed the title specs, add basic test case of clientInitialization cadl-ranch-specs, add basic test case of clientInitialization Sep 30, 2024
@tadelesh
Copy link
Member

@weidongxu-microsoft please hold this. i'm going to redesign all the client related thing in tcgc to consolidate all things related to clients, it may cause changes for decorator.

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Nov 14, 2024

@weidongxu-microsoft please hold this. i'm going to redesign all the client related thing in tcgc to consolidate all things related to clients, it may cause changes for decorator.

Sure.

Note that Face API already uses this decorator in their specs (and Java emitter also uses the .initialization in TCGC -- basically to put one ID path parameter to client parameter, in a subclient).
https://github.com/Azure/azure-rest-api-specs/blob/main/specification/ai/Face/client.tsp#L125

I guess .NET also uses the TCGC info.

I am fine if you want to break some, though.

Comment on lines +29 to +30
@route("/sub-client/{name}:action")
action(@path name: string): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, maybe add another parameter that will be taken in method signature, as a contrast?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I will first update to our local test first, as this one likely going to take time.
microsoft/typespec#5027

github-merge-queue bot pushed a commit to microsoft/typespec that referenced this pull request Nov 20, 2024
Link Azure/autorest.java#2972

Current limitation
- sub client need to be in same namespace of its parent.

Feature behind `enable-subclient` option (`false` means
client/operationGroup; `true` means all client, with client having
accessor to get subclient)

unbranded openai https://github.com/joseharriaga/openai-in-typespec

## Bug to be fixed in other PRs (relate to unbranded openai, but
unrelated to this feature -- moved to
#5047)
- #5026
- #5030
- #5028
- #5031

## Test

1. cadl-ranch PR Azure/cadl-ranch#727 (also
tried on test with more than 2 level subclient -- nested namespaces)
2. openai unbranded
https://github.com/joseharriaga/openai-in-typespec/compare/main...weidongxu-microsoft:java-sub-client?expand=1
([apiview](https://apiview.dev/Assemblies/Review/b974bacf316c495aa5c30ce0b7e07b1c/d1d7278c251d4805b189fd238714897d#com.openai.OpenAIClient))
3. Face API Azure/azure-sdk-for-java#42875
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.

5 participants