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

rename client class name for rlc #1349

Merged
merged 3 commits into from
Mar 23, 2022

Conversation

qiaozha
Copy link
Member

@qiaozha qiaozha commented Mar 22, 2022

Currently we are adding a Like suffix for client class name, and keep the title name as client factory name.
In this PR, we will add a Client suffix for client class name, if it's not end with Client. and use createClient as the client factory name. and remove the default export of client factory name for multi-clients.

@qiaozha qiaozha requested review from bterlson and xirzec March 22, 2022 11:04
Copy link
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this! Just a couple of nits about how the factories are exported.

@@ -884,6 +880,10 @@ export interface CascadeDeleteJobOutput {
status?: string;
}

// @public (undocumented)
function createClient(Endpoint: string, options?: ClientOptions): AzureAgriFoodPlatformDataPlaneServiceClient;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should only export this as the default export to leave for later adding a second client, what do you think @joheredi ?

Copy link
Member

Choose a reason for hiding this comment

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

I think this only exports it as the default below, there is no access to createClient as a named import/export

image

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if we could somehow make this clear in api-extractor or else api reviews are going to be misleading

Copy link
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

Left another comment after another pass

Copy link
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

Please change the export to as createClient for multi-client packages

@qiaozha qiaozha merged commit 1b89465 into Azure:main Mar 23, 2022
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.

None yet

3 participants