-
Notifications
You must be signed in to change notification settings - Fork 37
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]: SdkCredentialParameter should be limited to SdkCredentialType #1274
Comments
So the generic parameter here for union type is providing the lower boundary of those type variants, I cannot say I like it because in majority of cases, it is not really helpful. I kind of like a solution that we update the export interface SdkCredentialParameter extends SdkModelPropertyTypeBase {
kind: "credential";
type: SdkCredentialType;
onClient: true;
}
export interface SdkCredentialType extends SdkTypeBase {
kind: "credential";
supportedSchemes: HttpAuth[];
} It is a bigger breaking change, but I think this makes more sense. |
This comment was marked as outdated.
This comment was marked as outdated.
Here we have two options: export interface SdkCredentialParameter extends SdkModelPropertyTypeBase {
kind: "credential";
type: SdkCredentialType | SdkUnionType<SdkCredentialType>; // union of credentials
onClient: true;
} Option 2 export interface SdkCredentialType extends SdkTypeBase {
kind: "credential";
supportedSchemes: HttpAuth[];
} As we offline discussed Option 1 could better present the client type for credential information(credential is either a credential or a union of credentials). And Option 2 would explicitly present credential type but with implicit union meaning here, it is a list of supported schemas. So personally I would vote for Option 1 for a clear type information. @tadelesh @iscai-msft I notice we also talked about the endpoint parameter changes not sure there is similar issue with credential parameter. I'd like to hear your insight. |
@MaryGao I like option 1 as well. I think we can type the |
So
in the values we have:
Or you could think how the type
The generic parameter helps, but we still cannot remove the |
@iscai-msft Could you expand your idea of SdkUnionType<T1, ..., Tn> a little? I may be not aware that we have features to spread generic arugements(see issues). Back to Option 1, if we have possible values with SdkCredentialType or SdkEndpointType we may use |
In
Would result in an |
Request Change
Here is the definition of SdkCredentialParameter and it contains a union of
SdkUnionType
which will not be scoped intocredential
kind. But in fact any detailed type of crednetial parameter should becredential
kind. When traversing the union items we need to castcade to SdkCredentialType.Proposal
So my proposal is we could make SdkUnionType generic. And then in SdkCredentialParameter we could pass SdkCredentialType in union type.
With this change we could better narrow down the type info with credential only. But please note this would be a breaking change for emitters.
Checklist
The text was updated successfully, but these errors were encountered: