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

How to generate versions enum in modular? #2680

Closed
Tracked by #2828
MaryGao opened this issue Jul 18, 2024 · 4 comments · Fixed by #2889
Closed
Tracked by #2828

How to generate versions enum in modular? #2680

MaryGao opened this issue Jul 18, 2024 · 4 comments · Fixed by #2889
Assignees
Labels
HRLC p0 priority 0

Comments

@MaryGao
Copy link
Member

MaryGao commented Jul 18, 2024

This is from the discussion. With the case we have client-level api version defined.

@versioned(Versions)
@armCommonTypesVersion(CommonTypes.Versions.v5)
namespace Microsoft.EdgeZones;

/** Api versions */
enum Versions {
  /** 2024-04-01-preview api version */
  @useDependency(Azure.ResourceManager.Versions.v1_0_Preview_1)
  `2024-04-01-preview`,
}

Then we would generate string as client options. And also generating a fixed & orphan Versions enum from getAllModels with Usage.Version.

export interface EdgeZonesClientOptionalParams extends ClientOptions {
  /** The API version to use for this operation. */
  apiVersion?: string;
}
/** Api versions */
export type Versions = "2024-04-01-preview";

This may not be proper because the api version is an extensible enum which allows customers to pass a list outside the enums.

To solve this we could either ignore this value or generate as extensible enum

  • Ignore the version enum in Usage.Version in getAllModels since it's an orphen model
  • Generate as KnownVersions and link it to client option
export enum KnownVersions {
  /** 2024-04-01-preview */
  2024-04-01-preview = "2024-04-01-preview",
}

export interface EdgeZonesClientOptionalParams extends ClientOptions {
  /** The API version to use for this operation. 
   * {@link KnownVersions} 
  */
  apiVersion?: string;
}

Both are okay for me and personally i prefer generating as KnownVersions which could help suggest values in client side.

@qiaozha
Copy link
Member

qiaozha commented Jul 22, 2024

prefer to generate KnownVersions and update the document link in ClientOptions.

@joheredi
Copy link
Member

Can we just emit a fixed union and have users cast to any if they need to provide an unknown api version?

export interface EdgeZonesClientOptionalParams extends ClientOptions {
  /** The API version to use for this operation. */
  apiVersion?: "2024-04-01-preview";
}

@qiaozha qiaozha added p0 priority 0 and removed P1 priority 1 labels Jul 24, 2024
@MaryGao
Copy link
Member Author

MaryGao commented Jul 30, 2024

@joheredi Personally I prefer here to generate as string and with document enhancement for knownable values. Let me know how you think of this!

Firstly I'd like to clarify one thing here, there will be two concepts of api version,

  • one is apiVersion parameter which is defined as client-level or method-level parameter with type string in TCGC included in getAllOperations
  • another is Versions enum which is included in getAllModels in TCGC with Usage.Version

Regarding the types these two versions are independent with each other but considering only one client-leve apiVesion case, we are talking about - if we could be smart to indicate the possible values for that parameter. I think a weak linkage between them would be better with following considerations:

  • This would align with the principle generate what tsp defined
  • Building the direct linkage between them may mislead customers, because we only generate SDK with one selected apiVersion and there is no guarantee other listed apiversion could be supported well
  • Actually we met sererval issues from customers with apiVersions(see this and this) because the generated one is not working or other reasons, the more valuable information here would be set the correct workable apiVersion they may or may not list here. So I think string type is a better open door for them with changing default one.

@MaryGao
Copy link
Member Author

MaryGao commented Aug 5, 2024

Offline confired with Jose, we agreed to go with Option 2 - Generate as KnownVersions and link it to client option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HRLC p0 priority 0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants