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

http-client-java, accessor to subclient #5027

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

Conversation

weidongxu-microsoft
Copy link
Contributor

@weidongxu-microsoft weidongxu-microsoft commented Nov 8, 2024

@weidongxu-microsoft weidongxu-microsoft changed the title http-client-java. accessor to subclient http-client-java, accessor to subclient Nov 8, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the emitter:client:java Issue for the Java client emitter: @typespec/http-client-java label Nov 8, 2024
@azure-sdk
Copy link
Collaborator

No changes needing a change description found.

Comment on lines +597 to +604
const subClients = this.listSubClientsUnderClient(client, !enableSubclient);
if (enableSubclient) {
// subclient, no operation group
for (const subClient of subClients) {
const codeModelSubclient = this.processClient(subClient);
codeModelClient.addSubClient(codeModelSubclient);
}
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most is refactor (extract code to processClient, as we now need to call it recursively).

Here is the main change, with the option, we would not do operation group, and every client is a client (or subclient).

}

private listSubClientsUnderClient(
client: SdkClientType<SdkHttpOperation>,
includeNestedOperationGroups: boolean,
isRootClient: boolean,
Copy link
Contributor Author

@weidongxu-microsoft weidongxu-microsoft Nov 8, 2024

Choose a reason for hiding this comment

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

This isRootClient param seems not needed. If a client does not have parent, it is the root client.

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs

@@ -90,6 +92,8 @@ const EmitterOptionsSchema: JSONSchemaType<EmitterOptions> = {

"group-etag-headers": { type: "boolean", nullable: true },

"enable-subclient": { type: "boolean", nullable: true, default: false },
Copy link
Member

@haolingdong-msft haolingdong-msft Nov 8, 2024

Choose a reason for hiding this comment

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

Do we want to set the option to true in our test to see the impact? (at least for some of the tests)

Copy link
Contributor Author

@weidongxu-microsoft weidongxu-microsoft Nov 11, 2024

Choose a reason for hiding this comment

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

Yes, we'd plan to set it to true for the subclient related tests (and for the whole unbranded too, if we separate the flavors).

Currently my cadl-ranch PR is not merged, so we don't have them now. (test case, the basic one, is written before the http-specs package, I would need to migrate it first; and we will need more tests on advanced cases)

@weidongxu-microsoft weidongxu-microsoft marked this pull request as ready for review November 11, 2024 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
emitter:client:java Issue for the Java client emitter: @typespec/http-client-java
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants