-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[sdk/python] Support Sequence[T] for array types rather than List[T] #5282
Conversation
@@ -125,7 +127,7 @@ async def serialize_property(value: 'Input[Any]', | |||
remote_asset = cast('RemoteAsset', value) | |||
obj["uri"] = await serialize_property(remote_asset.uri, deps, input_transformer) | |||
else: | |||
raise AssertionError(f"unknown asset type: {value}") | |||
raise AssertionError(f"unknown asset type: {value!r}") |
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.
Otherwise mypy errors:
error: On Python 3 '{}'.format(b'abc') produces "b'abc'"; use !r if this is a desired behavior
due to the not isinstance(value, bytes)
above.
@@ -194,7 +196,7 @@ async def serialize_property(value: 'Input[Any]', | |||
value = _types.input_type_to_dict(value) | |||
transform_keys = False | |||
|
|||
if isinstance(value, Mapping): # pylint: disable=bad-option-value,isinstance-second-argument-not-valid-type | |||
if isinstance(value, abc.Mapping): |
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.
While the previous code worked, we should be checking against the abstract base class from collections.abc
rather than the type from typing
.
@@ -530,7 +550,7 @@ async def test_apply_does_not_propagate_secret_on_unknown_unknown_output_during_ | |||
settings.SETTINGS.dry_run = True | |||
|
|||
out = self.create_output(0, is_known=False) | |||
r = out.apply(lambda v: self.create_output("inner", is_known=false, is_secret=True)) | |||
r = out.apply(lambda v: self.create_output("inner", is_known=False, is_secret=True)) |
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.
Noticed this when I had the file open in VS Code with Pylance installed.
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.
But... how did it work before?
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 lambda passed to apply
is not called because settings.SETTINGS.dry_run = True
. If settings.SETTINGS.dry_run
was set to False
, then it fails as you'd expect with:
NameError: name 'false' is not defined
@@ -951,7 +988,18 @@ class InputTypeSerializationTests(unittest.TestCase): | |||
async def test_simple_input_type(self): | |||
it = FooArgs(first_arg="hello", second_arg=42) | |||
prop = await rpc.serialize_property(it, []) | |||
self.assertDictEqual(prop, {"firstArg": "hello", "secondArg": 42}) | |||
self.assertEqual({"firstArg": "hello", "secondArg": 42}, prop) |
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.
assertEqual
will use assertDictEqual
internally per https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertDictEqual
This method will be used by default to compare dictionaries in calls to assertEqual().
And for consistency elsewhere, specify the expected value first.
d53dcfa
to
3f76946
Compare
This is a non-breaking change. Python doesn’t have types at runtime, so nothing would be hard broken by the change. It is possible someone using We already did this for maps, though, and I haven't heard any feedback from users about it. Previously maps were annotated as I do wonder if we should consider annotating outputs (return types) as |
@@ -530,7 +550,7 @@ async def test_apply_does_not_propagate_secret_on_unknown_unknown_output_during_ | |||
settings.SETTINGS.dry_run = True | |||
|
|||
out = self.create_output(0, is_known=False) | |||
r = out.apply(lambda v: self.create_output("inner", is_known=false, is_secret=True)) | |||
r = out.apply(lambda v: self.create_output("inner", is_known=False, is_secret=True)) |
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.
But... how did it work before?
We currently emit array types as `List[T]` for Python, but `List[T]` is invariant, which causes type checkers like mypy to produce errors when values like `["foo", "bar"]` are passed as args typed as `List[pulumi.Input[str]]`. Instead, we should move to using `Sequence[T]` which is covariant, and does not have this problem. We actually already do this for `Dict` vs. `Mapping`, emitting map types as `Mapping[str, T]` rather than `Dict[str, T]` because `Mapping[str, T]` is covariant. This change makes us consistent for array types.
3f76946
to
cb63344
Compare
Similar to pulumi#5282, but for core SDK types. The tl;dr is that because Sequence[T] is covariant, constructing resources becomes much more ergonomic. Fix pulumi#7693.
Similar to pulumi#5282, but for core SDK types. The tl;dr is that because Sequence[T] is covariant, constructing resources becomes much more ergonomic. Fix pulumi#7693.
Similar to pulumi#5282, but for core SDK types. The tl;dr is that because Sequence[T] is covariant, constructing resources becomes much more ergonomic. Fix pulumi#7693.
We currently emit array types as
List[T]
for Python, butList[T]
is invariant, which causes type checkers like mypy to produce errors when values like["foo", "bar"]
are passed as args typed asList[pulumi.Input[str]]
(sinceInput[str]
is an alias forUnion[T, Awaitable[T], Output[T]]
. To address this, we should move to usingSequence[T]
which is covariant, and does not have this problem.We actually already do this for
Dict
vs.Mapping
, emitting map types asMapping[str, T]
rather thanDict[str, T]
becauseMapping[str, T]
is covariant for the value. This change makes us consistent for array types.These are the SDK changes necessary to support
Sequence[T]
.Codegen PR: #5283
Part of #5278