-
Notifications
You must be signed in to change notification settings - Fork 46
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] add namespace
flag
#2161
base: main
Are you sure you want to change the base?
Changes from all commits
daf8f54
e359988
0f77ea3
765887e
6863ed1
328778d
2b0a0b5
3099bce
fc46f80
57decd5
1f0cfb0
102a545
a234cf1
bde5252
143291b
ea0e91c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
--- | ||
changeKind: feature | ||
packages: | ||
- "@azure-tools/typespec-client-generator-core" | ||
--- | ||
|
||
Add support for `namespace` flag in tspconfig.yaml |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import { | |
DecoratorContext, | ||
DecoratorExpressionNode, | ||
DecoratorFunction, | ||
Diagnostic, | ||
Enum, | ||
EnumMember, | ||
Interface, | ||
|
@@ -17,6 +18,7 @@ import { | |
SyntaxKind, | ||
Type, | ||
Union, | ||
createDiagnosticCollector, | ||
getDiscriminator, | ||
getNamespaceFullName, | ||
getProjectedName, | ||
|
@@ -60,6 +62,7 @@ import { | |
AllScopes, | ||
clientNameKey, | ||
clientNamespaceKey, | ||
getRootUserDefinedNamespaceName, | ||
getValidApiVersion, | ||
isAzureCoreTspModel, | ||
negationScopesKey, | ||
|
@@ -1209,19 +1212,30 @@ export const $clientNamespace: ClientNamespaceDecorator = ( | |
export function getClientNamespace( | ||
context: TCGCContext, | ||
entity: Namespace | Interface | Model | Enum | Union, | ||
): string { | ||
): [string, readonly Diagnostic[]] { | ||
const diagnostics = createDiagnosticCollector(); | ||
const override = getScopedDecoratorData(context, clientNamespaceKey, entity); | ||
if (override) return override; | ||
if (override) { | ||
const rootNamespaceName = diagnostics.pipe(getRootUserDefinedNamespaceName(context)); | ||
// if you've defined a namespace flag, we replace the root name with the flag | ||
return diagnostics.wrap( | ||
override.replace(rootNamespaceName, context.namespace || rootNamespaceName), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. better to judge undefined of |
||
); | ||
} | ||
if (!entity.namespace) { | ||
return ""; | ||
return diagnostics.wrap(context.namespace || ""); | ||
} | ||
if (entity.kind === "Namespace") { | ||
return getNamespaceFullNameWithOverride(context, entity); | ||
} | ||
return getNamespaceFullNameWithOverride(context, entity.namespace); | ||
} | ||
|
||
function getNamespaceFullNameWithOverride(context: TCGCContext, namespace: Namespace): string { | ||
function getNamespaceFullNameWithOverride( | ||
context: TCGCContext, | ||
namespace: Namespace, | ||
): [string, readonly Diagnostic[]] { | ||
const diagnostics = createDiagnosticCollector(); | ||
const segments = []; | ||
let current: Namespace | undefined = namespace; | ||
while (current && current.name !== "") { | ||
|
@@ -1233,7 +1247,13 @@ function getNamespaceFullNameWithOverride(context: TCGCContext, namespace: Names | |
segments.unshift(current.name); | ||
current = current.namespace; | ||
} | ||
return segments.join("."); | ||
if (context.namespace) { | ||
const rootNamespaceName = diagnostics.pipe(getRootUserDefinedNamespaceName(context)); | ||
|
||
return diagnostics.wrap(segments.join(".").replace(rootNamespaceName, context.namespace)); | ||
} | ||
|
||
return diagnostics.wrap(segments.join(".")); | ||
} | ||
|
||
export const $scope: ScopeDecorator = ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,7 @@ import { | |
HttpOperationResponseContent, | ||
} from "@typespec/http"; | ||
import { getAddedOnVersions, getRemovedOnVersions, getVersions } from "@typespec/versioning"; | ||
import { getParamAlias } from "./decorators.js"; | ||
import { getParamAlias, listClients } from "./decorators.js"; | ||
import { | ||
DecoratorInfo, | ||
SdkBuiltInType, | ||
|
@@ -83,6 +83,8 @@ export function parseEmitterName( | |
} | ||
|
||
/** | ||
* | ||
* @deprecated Use `.namespaces` from sdkContext.sdkPackage instead | ||
* | ||
* @param context | ||
* @param namespace If we know explicitly the namespace of the client, pass this in | ||
|
@@ -593,3 +595,45 @@ export function getValueTypeValue( | |
return undefined; | ||
} | ||
} | ||
|
||
/** | ||
* Get all of the root user-defined namespaces in a TypeSpec definition if they're tied to a client | ||
* @param context | ||
* @returns | ||
*/ | ||
export function getRootUserDefinedNamespaceName( | ||
context: TCGCContext, | ||
): [string, readonly Diagnostic[]] { | ||
const diagnostics = createDiagnosticCollector(); | ||
const rootNamespaces = [...context.program.getGlobalNamespaceType().namespaces.values()]; | ||
tadelesh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// We explicitly want namespaces that are used to define a client, not namespaces that define the service of a client | ||
const clientNamespaces = listClients(context).map((x) => | ||
x.type.kind === "Namespace" ? x.type : x.type.namespace, | ||
); | ||
const globalNamespaces: string[] = []; | ||
for (const namespace of clientNamespaces) { | ||
const segments = []; | ||
let currNamespace: Namespace | undefined = namespace; | ||
while (currNamespace) { | ||
segments.unshift(currNamespace.name); | ||
if (rootNamespaces.includes(currNamespace)) { | ||
globalNamespaces.push(segments.join(".")); | ||
break; | ||
} | ||
currNamespace = currNamespace.namespace; | ||
} | ||
} | ||
// if we override with namespace flag, we should override the global namespace to the namespace flag | ||
if (globalNamespaces.length !== 1) { | ||
diagnostics.add( | ||
createDiagnostic({ | ||
code: "unclear-namespace-overriding", | ||
messageId: "default", | ||
format: {}, | ||
target: context.program.getGlobalNamespaceType(), | ||
}), | ||
); | ||
Comment on lines
+629
to
+636
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we just report there is multiple user defined ns? coupling this function with namespace override config seems not good. |
||
} | ||
return diagnostics.wrap(globalNamespaces[0] || ""); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing it but where are we blocking / ignoring this outside of azure flavor? @srnagar if I recall correctly that is what we were doing is azure only since thats the only use case that actually needs this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is we weren't going to block unbranded from using it, but we are really mainly adding it for Azure compatibility.
The general use case I have for this is:
main.tsp
that defines the service under one main namespace (saynamespace DocumentIntelligence
)client.tsp
with client-customizations under one main namespace (saynamespace DocumentIntelligenceCustomizations
)namespace
flag in tspconfig.yaml to give the azure name, sonamespace: Azure.DocumentIntelligence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
namespace
in tspconfig.yaml is mainly used for Azure and we will not use it for unbranded libraries. We just don't publicly documentnamespace
option for unbranded libraries.If we want to be very explicit, we can add a check in the TCGC that can be removed later if we want to extend this support for unbranded libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally feel like it's cleaner to keep it as is, and when we split up tcgc, we can keep
namespace
in thetcgc
library. I think I'd almost prefer if we only want azure to use it, to call it something likeazureNamespace
. I don't feel very strongly about this thoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree with just keeping it as is.