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

[tcgc] make endpoint overridable #1155

Merged
merged 15 commits into from
Aug 6, 2024
Merged

[tcgc] make endpoint overridable #1155

merged 15 commits into from
Aug 6, 2024

Conversation

iscai-msft
Copy link
Contributor

@iscai-msft iscai-msft commented Jul 11, 2024

fixes #1153

@azure-sdk
Copy link
Collaborator

azure-sdk commented Jul 11, 2024

All changed packages have been documented.

  • @azure-tools/typespec-client-generator-core
Show changes

@azure-tools/typespec-client-generator-core - feature ✏️

Make literal endpoints overridable

@azure-sdk
Copy link
Collaborator

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft left a comment

Choose a reason for hiding this comment

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

LGTM

packages/typespec-client-generator-core/src/package.ts Outdated Show resolved Hide resolved
@iscai-msft
Copy link
Contributor Author

For this PR, I'm going to hold on merging it before this release because there's still open discussion about what the best way to do this is. In the meantime, I want to confirm that no language is blocked right now, correct? It's just that emitters are having to hardcode information about ARM.

@haolingdong-msft
Copy link
Member

haolingdong-msft commented Jul 16, 2024

For this PR, I'm going to hold on merging it before this release because there's still open discussion about what the best way to do this is. In the meantime, I want to confirm that no language is blocked right now, correct? It's just that emitters are having to hardcode information about ARM.

Hi @iscai-msft, I'm afraid it blocks Java adopting TCGC sdkpackage, I guess it also blocks other languages, as it will impact all management-plane generated SDKs, it causes public API breaking change. So if TCGC will not do this and we would like to adopt TCGC sdkpackage, each language emitter will do their own handling.

API change if we do not make endpoint overridable.
image

@iscai-msft
Copy link
Contributor Author

@haolingdong-msft can java do their own handling for this release? I would use this code snippet:

function serializeClient(context: SdkContext, client: SdkClientType) {
  ...
  for (param of client.initialization.parameters) {
    if (param.kind === "endpoint" && context.arm) {
      param.clientDefaultValue = param.serverUrl;
      param.serverUrl = "{baseUrl}";
      param.templateArguments = [{
          "baseUrl",
          isGeneratedName: true,
          description: "Service host",
          kind: "path",
          onClient: true,
          urlEncode: false,
          optional: false,
          serializedName: "baseUrl",
          correspondingMethodParams: [],
          type: {
            kind: "string",
            encode: "string",
            decorators: [],
          },
          isApiVersionParam: false,
          apiVersions: context.__tspTypeToApiVersions.get(client.type)!,
          crossLanguageDefinitionId: `${getCrossLanguageDefinitionId(context, client.service)}.baseUrl`,
          decorators: [],
        },
      ]
    }
  }
}

function mainFunc(context: SdkContext) {
  for (const client of sdkPackage.clients) {
    serializeClient(context, client);
  }
}

We just want to make sure everyone's on the same page before committing this to tcgc. Agree that this isn't very nice code, but with this it shouldn't be blocking

@haolingdong-msft
Copy link
Member

haolingdong-msft commented Jul 17, 2024

templateArguments 

It's fine, I will do the handling in Java emitter first. Please let me know the decission of this issue. Thanks

@tadelesh
Copy link
Member

@iscai-msft is there any update for this? for typespec like follows, it seems there is no default value from current templated argument

@server(
  "{endpoint}",
  "Test server endpoint",
  {
    endpoint: string = "http://localhost:3000",
  }
)

@iscai-msft
Copy link
Contributor Author

@iscai-msft is there any update for this? for typespec like follows, it seems there is no default value from current templated argument

@server(
  "{endpoint}",
  "Test server endpoint",
  {
    endpoint: string = "http://localhost:3000",
  }
)

that's a separate issue that i've fixed in a pr, following up on this current issue right now

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs

@iscai-msft iscai-msft added this pull request to the merge queue Aug 6, 2024
Merged via the queue into main with commit 4a5c0fb Aug 6, 2024
22 checks passed
@iscai-msft iscai-msft deleted the default_endpoint branch August 6, 2024 17:08
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.

Always make the baseUrl overridable in clients
7 participants