Skip to content

Commit

Permalink
fix(python): accept Sequence[T] for array parameters (#2606)
Browse files Browse the repository at this point in the history
Instead of typing array (list) parameters as `typing.List[T]`, using the
more flexible `typing.Sequence[T]` type, as recommended by the `typing`
package documentation for this use-case.

Using `List[T]` is inconvenient as it is invariant ([ref]), and requires
very intentional typing, which elads to boilerplate. `Sequence[T]` is
variant, which removes the need for boilerplate, as long as all items
in the sequence match the expected type.

[ref]: https://mypy.readthedocs.io/en/latest/common_issues.html#variance

This was reported in aws/aws-cdk#13203



---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
  • Loading branch information
RomainMuller authored Mar 31, 2021
1 parent c32a889 commit b09d578
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 21 deletions.
15 changes: 12 additions & 3 deletions packages/jsii-pacmak/lib/targets/python.ts
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,10 @@ abstract class BaseMethod implements PythonBase {
liftedPropNames,
);

const paramType = toTypeName(param).pythonType(context);
const paramType = toTypeName(param).pythonType({
...context,
parameterType: true,
});
const paramDefault = param.optional ? ' = None' : '';

pythonParams.push(`${paramName}: ${paramType}${paramDefault}`);
Expand Down Expand Up @@ -565,7 +568,10 @@ abstract class BaseMethod implements PythonBase {
// Iterate over all of our props, and reflect them into our params.
for (const prop of liftedProperties) {
const paramName = toPythonParameterName(prop.name);
const paramType = toTypeName(prop).pythonType(context);
const paramType = toTypeName(prop).pythonType({
...context,
parameterType: true,
});
const paramDefault = prop.optional ? ' = None' : '';

pythonParams.push(`${paramName}: ${paramType}${paramDefault}`);
Expand Down Expand Up @@ -1203,7 +1209,10 @@ class StructField implements PythonBase {

public constructorDecl(context: EmitContext) {
const opt = this.optional ? ' = None' : '';
return `${this.pythonName}: ${this.typeAnnotation(context)}${opt}`;
return `${this.pythonName}: ${this.typeAnnotation({
...context,
parameterType: true,
})}${opt}`;
}

/**
Expand Down
10 changes: 9 additions & 1 deletion packages/jsii-pacmak/lib/targets/python/type-name.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,13 @@ export interface NamingContext {
* or not when emitting type signatures.
*/
readonly emittedTypes: Set<string>;

/**
* Whether the type is emitted for a parameter or not. This may change the
* exact type signature being emitted (e.g: Arrays are typing.Sequence[T] for
* parameters, and typing.List[T] otherwise).
*/
readonly parameterType?: boolean;
}

export function toTypeName(ref?: OptionalValue | TypeReference): TypeName {
Expand Down Expand Up @@ -160,7 +167,8 @@ class List implements TypeName {
}

public pythonType(context: NamingContext) {
return `typing.List[${this.#element.pythonType(context)}]`;
const type = context.parameterType ? 'Sequence' : 'List';
return `typing.${type}[${this.#element.pythonType(context)}]`;
}

public requiredImports(context: NamingContext) {
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/jsii-pacmak/test/generated-code/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export function verifyGeneratedCodeFor(

expect({ [TREE]: checkTree(outDir) }).toMatchSnapshot('<outDir>/');

if (targetName !== TargetName.PYTHON) {
if (targetName !== TargetName.PYTHON || process.env.SKIP_MYPY_CHECK) {
return Promise.resolve();
}
return runMypy(path.join(outDir, targetName));
Expand Down

0 comments on commit b09d578

Please sign in to comment.