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

Mgmt rest client #47665

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

Mgmt rest client #47665

wants to merge 13 commits into from

Conversation

live1206
Copy link
Member

@live1206 live1206 commented Dec 26, 2024

Resolves #45830

  • Update clients for MPG
    • public to internal
    • output file path update
  • Transform SubscriptionId to method parameter and type to string

@github-actions github-actions bot added Client This issue points to a problem in the data-plane of the library. CodeGen Issues that relate to code generation labels Dec 26, 2024
{
type.Update(modifiers: TransfromModifiers(type), relativeFilePath: TransformRelativeFilePathForClient(type));
}
//if (type is RestClientProvider)
Copy link
Member Author

@live1206 live1206 Dec 26, 2024

Choose a reason for hiding this comment

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

If we make both client and restclient internal, all the models referenced will be internalized during the postprocess.
Once we have the resource and collection in place, public reference will be in place and the referenced models will be kept public.
So, keep restclient public for now to make the referenced models public.

Copy link
Member

Choose a reason for hiding this comment

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

could we put this comment as a comment in the code? just in case we could remember it and enable this piece in the future when we have the resource/collection types to keep those models public

Copy link
Member Author

Choose a reason for hiding this comment

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

Added TODO

@live1206 live1206 marked this pull request as ready for review December 26, 2024 07:08
@live1206 live1206 added the Mgmt This issue is related to a management-plane library. label Dec 26, 2024
private static string TransformRelativeFilePathForRestClient(TypeProvider type)
=> Path.Combine("src", "Generated", "RestOperations", $"{type.Name}.RestClient.cs");

private static TypeSignatureModifiers TransfromModifiers(TypeProvider type)
Copy link
Member

Choose a reason for hiding this comment

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

nit: should this be: TransformPublicToInternal or similar to be explicit

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed

{
if (parameter.NameInRequest.Equals("subscriptionId", StringComparison.OrdinalIgnoreCase))
{
parameters.Add(new InputParameter(parameter.Name, parameter.NameInRequest, parameter.Summary, parameter.Doc, InputPrimitiveType.String, parameter.Location, parameter.DefaultValue, InputOperationParameterKind.Method, parameter.IsRequired, parameter.IsApiVersion, parameter.IsResourceParameter, parameter.IsContentType, parameter.IsEndpoint, parameter.SkipUrlEncoding, parameter.Explode, parameter.ArraySerializationDelimiter, parameter.HeaderCollectionPrefix));
Copy link
Member

Choose a reason for hiding this comment

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

do we really need to change the type of subscriptionId back to string?
We do this in autorest.csharp because we want to minimize the changes to the old generator, since this is a new generator, we could cover the case that subscriptionId is a guid

Copy link
Member Author

@live1206 live1206 Jan 3, 2025

Choose a reason for hiding this comment

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

not quite following here, change subscriptionId type will break the public contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. CodeGen Issues that relate to code generation Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate RestClients for Azure services with Azure Plugin
3 participants