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

Python multi namespace #5375

Open
wants to merge 66 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
feb80b5
update dependency tcgc 0.48.1
msyyc Nov 11, 2024
60e4c15
add clientNamespace property
msyyc Nov 11, 2024
ac2b225
init
msyyc Nov 25, 2024
0a391bd
init2
msyyc Nov 26, 2024
80ea470
init3
msyyc Nov 27, 2024
d7f6e87
init4
msyyc Dec 2, 2024
4146dc4
init5
msyyc Dec 2, 2024
f0d01be
format
msyyc Dec 3, 2024
3613073
format
msyyc Dec 3, 2024
db6402e
merge
msyyc Dec 3, 2024
5ea817c
review
msyyc Dec 3, 2024
ed61e31
review
msyyc Dec 3, 2024
682b381
review
msyyc Dec 4, 2024
32399e8
review
msyyc Dec 4, 2024
ab818e2
review
msyyc Dec 4, 2024
f3f0140
review
msyyc Dec 4, 2024
ea246a9
review
msyyc Dec 4, 2024
08a68ff
review
msyyc Dec 5, 2024
a8dc1d4
review
msyyc Dec 5, 2024
089ebcc
review
msyyc Dec 5, 2024
59d97c2
review
msyyc Dec 6, 2024
bbb5884
Merge branch 'main' of https://github.com/microsoft/typespec into pyt…
msyyc Dec 6, 2024
9c5f209
review
msyyc Dec 6, 2024
d8881d1
review
msyyc Dec 6, 2024
5aeb9cf
review
msyyc Dec 6, 2024
c8cac2c
review
msyyc Dec 6, 2024
b0b1b43
review
msyyc Dec 6, 2024
a21d7ad
review
msyyc Dec 6, 2024
6c37c0c
review
msyyc Dec 6, 2024
d0dcc68
review
msyyc Dec 6, 2024
6121243
merge
msyyc Dec 6, 2024
05309a8
for sample and test
msyyc Dec 6, 2024
f3d25ff
for sample and test
msyyc Dec 6, 2024
42f29c9
review
msyyc Dec 6, 2024
bf9806d
merge
msyyc Dec 9, 2024
ab88321
debug
msyyc Dec 9, 2024
7f88e3e
review
msyyc Dec 9, 2024
adf95fe
fix
msyyc Dec 10, 2024
7c5532c
fix
msyyc Dec 10, 2024
2781b09
new test
msyyc Dec 10, 2024
657176d
review
msyyc Dec 10, 2024
29c30fd
review
msyyc Dec 10, 2024
af457ca
review
msyyc Dec 10, 2024
b58ef98
review
msyyc Dec 10, 2024
5458bb5
merge
msyyc Dec 11, 2024
d5b473a
fix
msyyc Dec 11, 2024
689a06b
fix
msyyc Dec 11, 2024
4128461
fix
msyyc Dec 11, 2024
08ca7c7
Merge branch 'main' of https://github.com/microsoft/typespec into pyt…
msyyc Dec 13, 2024
6aa4203
fix typing
msyyc Dec 13, 2024
5700597
fix typing
msyyc Dec 13, 2024
49cc526
review
msyyc Dec 13, 2024
5b7fe1a
review
msyyc Dec 13, 2024
7290a71
Merge branch 'main' of https://github.com/microsoft/typespec into pyt…
msyyc Dec 16, 2024
c592198
review
msyyc Dec 16, 2024
21d8448
review
msyyc Dec 16, 2024
851fd6c
fix crash when no og in client
msyyc Dec 23, 2024
b493479
review
msyyc Dec 23, 2024
0449f59
Merge branch 'main' of https://github.com/microsoft/typespec into pyt…
msyyc Dec 23, 2024
85435c3
review
msyyc Dec 23, 2024
c8cd8d1
review
msyyc Dec 24, 2024
a9bdfa7
fix import
msyyc Dec 24, 2024
06f42b6
fix import
msyyc Dec 24, 2024
34d52e5
review
msyyc Dec 25, 2024
87d391e
review
msyyc Dec 25, 2024
8f50702
format
msyyc Dec 25, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions packages/http-client-python/emitter/src/code-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@ import {
simpleTypesMap,
typesMap,
} from "./types.js";
import { emitParamBase, getImplementation, removeUnderscoresFromNamespace } from "./utils.js";
import {
emitParamBase,
getClientNamespace,
getImplementation,
removeUnderscoresFromNamespace,
} from "./utils.js";

function emitBasicMethod<TServiceOperation extends SdkServiceOperation>(
context: PythonSdkContext<TServiceOperation>,
Expand Down Expand Up @@ -193,6 +198,7 @@ function emitOperationGroups<TServiceOperation extends SdkServiceOperation>(
propertyName: operationGroup.name,
operations: operations,
operationGroups: emitOperationGroups(context, operationGroup, rootClient, name),
clientNamespace: getClientNamespace(context, operationGroup.clientNamespace),
});
}
}
Expand All @@ -210,10 +216,18 @@ function emitOperationGroups<TServiceOperation extends SdkServiceOperation>(
className: "",
propertyName: "",
operations: operations,
clientNamespace: getClientNamespace(context, client.clientNamespace),
});
}
}

// operation has same clientNamespace as the operation group
for (const og of operationGroups) {
for (const op of og.operations) {
op.clientNamespace = getClientNamespace(context, og.clientNamespace);
}
}

return operationGroups.length > 0 ? operationGroups : undefined;
}

Expand Down Expand Up @@ -247,6 +261,7 @@ function emitClient<TServiceOperation extends SdkServiceOperation>(
url,
apiVersions: client.apiVersions,
arm: context.arm,
clientNamespace: getClientNamespace(context, client.clientNamespace),
};
}

Expand All @@ -268,14 +283,9 @@ export function emitCodeModel<TServiceOperation extends SdkServiceOperation>(
const codeModel: Record<string, any> = {
namespace: removeUnderscoresFromNamespace(sdkPackage.rootNamespace).toLowerCase(),
clients: [],
subnamespaceToClients: {},
};
for (const client of sdkPackage.clients) {
codeModel["clients"].push(emitClient(sdkContext, client));
if (client.nameSpace === sdkPackage.rootNamespace) {
} else {
codeModel["subnamespaceToClients"][client.nameSpace] = emitClient(sdkContext, client);
}
}
// loop through models and enums since there may be some orphaned models needs to be generated
for (const model of sdkPackage.models) {
Expand Down
4 changes: 4 additions & 0 deletions packages/http-client-python/emitter/src/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ export interface PythonEmitterOptions {
debug?: boolean;
flavor?: "azure";
"examples-dir"?: string;
// If true, package namespace will respect the typespec namespace. Otherwise,
// package namespace is always aligned with package name.
"enable-typespec-namespace"?: boolean;
"use-pyodide"?: boolean;
}

Expand Down Expand Up @@ -44,6 +47,7 @@ const EmitterOptionsSchema: JSONSchemaType<PythonEmitterOptions> = {
debug: { type: "boolean", nullable: true },
flavor: { type: "string", nullable: true },
"examples-dir": { type: "string", nullable: true, format: "absolute-path" },
"enable-typespec-namespace": { type: "boolean", nullable: true },
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we add some comment for the new config? i believe we should do such clean up with the pr of consolidating emitter's config name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment for the new flag. And I talked to JS/.NET devs, then they are fine for current name.

"use-pyodide": { type: "boolean", nullable: true },
},
required: [],
Expand Down
20 changes: 16 additions & 4 deletions packages/http-client-python/emitter/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,13 @@ import { Type } from "@typespec/compiler";
import { HttpAuth, Visibility } from "@typespec/http";
import { dump } from "js-yaml";
import { PythonSdkContext } from "./lib.js";
import { camelToSnakeCase, emitParamBase, getAddedOn, getImplementation } from "./utils.js";
import {
camelToSnakeCase,
emitParamBase,
getAddedOn,
getClientNamespace,
getImplementation,
} from "./utils.js";

export const typesMap = new Map<SdkType, Record<string, any>>();
export const simpleTypesMap = new Map<string | null, Record<string, any>>();
Expand Down Expand Up @@ -75,7 +81,7 @@ export function getType<TServiceOperation extends SdkServiceOperation>(
case "union":
return emitUnion(context, type);
case "enum":
return emitEnum(type);
return emitEnum(context, type);
case "constant":
return emitConstant(type)!;
case "array":
Expand All @@ -86,7 +92,7 @@ export function getType<TServiceOperation extends SdkServiceOperation>(
case "duration":
return emitDurationOrDateType(type);
case "enumvalue":
return emitEnumMember(type, emitEnum(type.enumType));
return emitEnumMember(type, emitEnum(context, type.enumType));
case "credential":
return emitCredential(type);
case "bytes":
Expand Down Expand Up @@ -289,6 +295,7 @@ function emitModel<TServiceOperation extends SdkServiceOperation>(
usage: type.usage,
isXml: type.usage & UsageFlags.Xml ? true : false,
xmlMetadata: type.usage & UsageFlags.Xml ? getXmlMetadata(type) : undefined,
clientNamespace: getClientNamespace(context, type.clientNamespace),
};

typesMap.set(type, newValue);
Expand All @@ -314,7 +321,10 @@ function emitModel<TServiceOperation extends SdkServiceOperation>(
return newValue;
}

function emitEnum(type: SdkEnumType): Record<string, any> {
function emitEnum<TServiceOperation extends SdkServiceOperation>(
context: PythonSdkContext<TServiceOperation>,
type: SdkEnumType,
): Record<string, any> {
if (typesMap.has(type)) {
return typesMap.get(type)!;
}
Expand Down Expand Up @@ -352,6 +362,7 @@ function emitEnum(type: SdkEnumType): Record<string, any> {
values,
xmlMetadata: {},
crossLanguageDefinitionId: type.crossLanguageDefinitionId,
clientNamespace: getClientNamespace(context, type.clientNamespace),
};
for (const value of type.values) {
newValue.values.push(emitEnumMember(value, newValue));
Expand Down Expand Up @@ -469,6 +480,7 @@ function emitUnion<TServiceOperation extends SdkServiceOperation>(
type: "combined",
types: type.variantTypes.map((x) => getType(context, x)),
xmlMetadata: {},
clientNamespace: getClientNamespace(context, type.clientNamespace),
});
}

Expand Down
27 changes: 27 additions & 0 deletions packages/http-client-python/emitter/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,30 @@ export function isAzureCoreErrorResponse(t: SdkType | undefined): boolean {
export function capitalize(name: string): string {
return name[0].toUpperCase() + name.slice(1);
}

export function getClientNamespace<TServiceOperation extends SdkServiceOperation>(
context: PythonSdkContext<TServiceOperation>,
clientNamespace: string,
) {
const rootNamespace = removeUnderscoresFromNamespace(
context.sdkPackage.rootNamespace,
).toLowerCase();
if ([undefined, false].includes(context.emitContext.options["enable-typespec-namespace"])) {
return rootNamespace;
}
if (
[
"azure.core",
"azure.resourcemanager",
"azure.clientgenerator.core",
"typespec.rest",
"typespec.http",
"typespec.versioning",
].some((item) => clientNamespace.toLowerCase().startsWith(item))
) {
return rootNamespace;
}
return clientNamespace === ""
? rootNamespace
: removeUnderscoresFromNamespace(clientNamespace).toLowerCase();
}
3 changes: 3 additions & 0 deletions packages/http-client-python/eng/scripts/ci/regenerate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ const EMITTER_OPTIONS: Record<string, Record<string, string> | Record<string, st
"client/structure/two-operation-group": {
"package-name": "client-structure-twooperationgroup",
},
"client/namespace": {
"enable-typespec-namespace": "true",
},
Comment on lines +125 to +127
Copy link
Member

Choose a reason for hiding this comment

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

for unbranded. shall we always enable it by default? i heard java do that by default and for unbranded, it is reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is generated code for e2e demo allenjzhang/typespec-e2e-demo#51 if we enable typespec namespace. We can see there are additional levels of package structure, which is not intuitive. If SDK users want to call some API, they just need to import client then call it:

from todo import TodoAppClient

client = TodoAppClient()
client.users.get(...)

However, if they need models, they have to make it clear where the model is:

from todo.models import CreateFormResponse
from todo.users.models import UserExistsResponse

Compared with before that users can always import any model from todo.models, the usage experience is not better after enable typespec namespace. So I think we shall default --enable-typepsec-namespace as False which I think is better for most developers. And if typespec is really complicated and typepspec author really want the multinamespace in Python SDK, they could add the flag as True in tspconfig.yaml.

Copy link
Member

@tadelesh tadelesh Dec 25, 2024

Choose a reason for hiding this comment

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

i think it is because python does not support initialize the users client directly. to me, users should be a separate client and should be put under the todo.users, then the client, operation, model could be all under todo.users. i prefer to honor the typespec namespace all the time because it could reflect all the spec author's idea. any weird structure should come from a poor spec and author should fix the spec directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iscai-msft / @lmazuel for awareness.

};

function toPosix(dir: string): string {
Expand Down
2 changes: 1 addition & 1 deletion packages/http-client-python/generator/pygen/black.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def process(self) -> bool:
"venv",
"env",
)
and not Path(f).parts[0].startswith(".")
and (not Path(f).parts[0].startswith(".") or Path(f).parts[0] == "..")
and Path(f).suffix == ".py"
],
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class OptionsRetriever:
"generate-test": False,
"from-typespec": False,
"emit-cross-language-definition-file": False,
"enable-typespec-namespace": False,
}

@property
Expand Down Expand Up @@ -244,7 +245,7 @@ def _validate_code_model_options(self) -> None:
@staticmethod
def sort_exceptions(yaml_data: Dict[str, Any]) -> None:
for client in yaml_data["clients"]:
for group in client["operationGroups"]:
for group in client.get("operationGroups", []):
for operation in group["operations"]:
if not operation.get("exceptions"):
continue
Expand All @@ -261,7 +262,7 @@ def sort_exceptions(yaml_data: Dict[str, Any]) -> None:
@staticmethod
def remove_cloud_errors(yaml_data: Dict[str, Any]) -> None:
for client in yaml_data["clients"]:
for group in client["operationGroups"]:
for group in client.get("operationGroups", []):
for operation in group["operations"]:
if not operation.get("exceptions"):
continue
Expand Down Expand Up @@ -316,6 +317,7 @@ def _build_code_model_options(self) -> Dict[str, Any]:
"flavor",
"company_name",
"emit_cross_language_definition_file",
"enable_typespec_namespace",
]
return {f: getattr(self.options_retriever, f) for f in flags}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,7 @@
TYPESPEC_PACKAGE_MODE = ["azure-mgmt", "azure-dataplane", "generic"]
VALID_PACKAGE_MODE = SWAGGER_PACKAGE_MODE + TYPESPEC_PACKAGE_MODE
NAME_LENGTH_LIMIT = 40


def get_parent_namespace(namespace: str) -> str:
return namespace.rsplit(".", 1)[0]
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,7 @@ def xml_serialization_ctxt(self) -> Optional[str]:
attrs_list.append("'text': True")
return ", ".join(attrs_list)

@property
def serialization_type(self) -> str:
def serialization_type(self, **kwargs: Any) -> str:
"""The tag recognized by 'msrest' as a serialization/deserialization.

'str', 'int', 'float', 'bool' or
Expand All @@ -103,7 +102,7 @@ def serialization_type(self) -> str:

@property
def msrest_deserialization_key(self) -> str:
return self.serialization_type
return self.serialization_type()

@property
def client_default_value(self) -> Any:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ def __init__(
self.api_versions: List[str] = yaml_data["apiVersions"]
self.added_on: Optional[str] = yaml_data.get("addedOn")
self.external_docs: Optional[Dict[str, Any]] = yaml_data.get("externalDocs")
self.client_namespace: str = yaml_data.get("clientNamespace", code_model.namespace)

if code_model.options["version_tolerant"] and yaml_data.get("abstract"):
_LOGGER.warning(
Expand Down Expand Up @@ -112,7 +113,7 @@ def description(self) -> str:
)
return self._description or self.name

def method_signature(self, async_mode: bool) -> List[str]:
def method_signature(self, async_mode: bool, **kwargs: Any) -> List[str]:
if self.abstract:
return ["*args,", "**kwargs"]
return self.parameters.method_signature(async_mode)
return self.parameters.method_signature(async_mode, **kwargs)
Loading
Loading