feat(python): idiomatic capitalization for structs by rix0rrr · Pull Request #586 · aws/jsii

rix0rrr · 2019-07-05T11:32:40Z

This change addresses 4 issues:

Structs now use idiomatic (snake_case) capitalization of fields
(instead of JSII-inherited camelCase).
IDE support -- replace TypedDict usage with regular classes. This
makes it so that we can't use dicts anymore, but mypy support in
IDEs wasn't great and by using POPOs (Plain Old Python Objects)
IDEs get their support back.
Structs in a variadic argument use to be incorrectly lifted to
keyword arguments, this no longer happens.
Stop emitting "Stable" stabilities in docstrings, "Stable" is implied.

In order to make this change, I've had to make jsii-pacmak depend on
jsii-reflect. This is the proper layering of libraries anyway, since
jsii-reflect adds much-needed--and otherwise
duplicated--interpretation of the spec.

Complete refactoring of jsii-pacmak to build on jsii-reflect is too
much work right now, however, so I've added "escape hatching" where
generators can take advantage of the power of jsii-reflect if they want
to, but most of the code still works on the raw spec level.

Added a refactoring where we load the assembly once and reuse the same
instance for all generators, instead of loading the assembly for every
generator. Assembly-loading, especially with a lot of dependencies, takes
a non-negligible amount of time, so this has the side effect of making
the packaging step faster (shaves off 100 packages * 3 targets * a
couple of seconds).

Fixes #537
Fixes #577
Fixes #578
Fixes #588

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

This change addresses 4 issues: - Structs now use idiomatic (snake_case) capitalization of fields (instead of JSII-inherited camelCase). - IDE support -- replace TypedDict usage with regular classes. This makes it so that we can't use dicts anymore, but mypy support in IDEs wasn't great and by using POPOs (Plain Old Python Objects) IDEs get their support back. - Structs in a variadic argument use to be incorrectly lifted to keyword arguments, this no longer happens. - Stop emitting "Stable" stabilities in docstrings, "Stable" is implied. In order to make this change, I've had to make `jsii-pacmak` depend on `jsii-reflect`. This is the proper layering of libraries anyway, since `jsii-reflect` adds much-needed--and otherwise duplicated--interpretation of the spec. Complete refactoring of `jsii-pacmak` to build on `jsii-reflect` is too much work right now, however, so I've added "escape hatching" where generators can take advantage of the power of jsii-reflect if they want to, but most of the code still works on the raw spec level. Added a refactoring where we load the assembly once and reuse the same instance for all generators, instead of loading the assembly for every generator. Assembly-loading, especially with a lot of dependencies, takes a non-negligible amount of time, so this has the side effect of making the packaging step faster (shaves off 100 packages * 3 targets * a couple of seconds).

rix0rrr · 2019-07-05T11:33:34Z

For reference, structs now look like this:

@jsii.data_type(jsii_type="jsii-calc.TopLevelStruct", jsii_struct_bases=[], name_mapping={'required': 'required', 'second_level': 'secondLevel', 'optional': 'optional'})
class TopLevelStruct:
    def __init__(self, *, required: str, second_level: typing.Union[jsii.Number, "SecondLevelStruct"], optional: typing.Optional[str]=None):
        """
        Arguments:
            required: This is a required field.
            second_level: A union to really stress test our serialization.
            optional: You don't have to pass this.

        Stability:
            experimental
        """
        self._values = {
            'required': required,
            'second_level': second_level,
        }
        if optional is not None: self._values["optional"] = optional

    @property
    def required(self) -> str:
        """This is a required field.

        Stability:
            experimental
        """
        return self._values.get('required')

    @property
    def second_level(self) -> typing.Union[jsii.Number, "SecondLevelStruct"]:
        """A union to really stress test our serialization.

        Stability:
            experimental
        """
        return self._values.get('second_level')

    @property
    def optional(self) -> typing.Optional[str]:
        """You don't have to pass this.

        Stability:
            experimental
        """
        return self._values.get('optional')

    def _to_jsii(self) -> dict:
        return {
          'required': self.required,
          'secondLevel': self.second_level,
          'optional': self.optional,
        }

    def __eq__(self, rhs) -> bool:
        return isinstance(rhs, self.__class__) and rhs._values == self._values

    def __ne__(self, rhs) -> bool:
        return not (rhs == self)

    def __repr__(self) -> str:
        return 'TopLevelStruct(%s)' % ', '.join(k + '=' + repr(v) for k, v in self._values.items())

rix0rrr · 2019-07-05T11:51:45Z

And a jsii function now looks like this:

    @jsii.member(jsii_name="roundTrip")
    @classmethod
    def round_trip(cls, _positional: jsii.Number, *, required: str, second_level: typing.Union[jsii.Number, "SecondLevelStruct"], optional: typing.Optional[str]=None) -> "TopLevelStruct":
        """
        Arguments:
            _positional: -
            input: -
            required: This is a required field.
            second_level: A union to really stress test our serialization.
            optional: You don't have to pass this.

        Stability:
            experimental
        """
        input = TopLevelStruct(required=required, second_level=second_level, optional=optional)

        return jsii.sinvoke(cls, "roundTrip", jsii.structs_to_jsii([_positional, input]))

… with unmapping Remove now-unused dependency on mypy_extensions.

garnaat · 2019-07-05T13:15:05Z

    def wrapped(kernel, *args, **kwargs):
-        return _recursize_dereference(kernel, fn(kernel, *args, **kwargs))
+        output = fn(kernel, *args, **kwargs)
+        print(output)


probably don't want to leave this in there

rix0rrr · 2019-07-05T15:05:40Z

The last commit, switching to ReST-style docstrings (in order to help IDEs out, poor things!) is speculative.

It might be better with, or without. Any reviewers/mergers/editors of this PR should feel fully free to drop and/or amend it :).

garnaat · 2019-07-05T19:45:11Z

There was a problem with ctors that take no parameters. It was getting generated as:

class foo():
    def __init__(self, *):

but you can't have a bare asterisk that isn't followed by any params. I made this change:

diff --git a/packages/jsii-pacmak/lib/targets/python.ts b/packages/jsii-pacmak/lib/targets/python.ts
index a1a89786..ebd0131c 100644
--- a/packages/jsii-pacmak/lib/targets/python.ts
+++ b/packages/jsii-pacmak/lib/targets/python.ts
@@ -656,8 +656,13 @@ class Struct extends BasePythonClassType {
 
     private emitConstructor(code: CodeMaker, resolver: TypeResolver) {
         const members = this.allMembers;
+        let constructorArguments: string[] = [];
 
-        const constructorArguments = ['self', '*', ...members.map(m => m.constructorDecl(resolver))];
+        if (members.length > 0) {
+            constructorArguments = ['self', '*', ...members.map(m => m.constructorDecl(resolver))];
+        } else {
+            constructorArguments = ['self'];
+        }

and tested it and it worked. There may be a better way to do it in TypeScript. I don't think I can push the change here myself.

Common mixed-case abbrevations will be snake-cased as a while, instead of being broken up into separate word parts.

eladb

I think we need support for struct inheritance...

eladb · 2019-07-07T09:10:40Z

+//
+// These functions exist to break the isolation barrier until we have time
+// to rewrite the code to properly use jsii-reflect.
+


Why not just expose spec as public in Jsii-reflect?

I went down that path first, but I think it's the wrong way round. Don't feel that spec should be exposed.

eladb · 2019-07-07T09:38:08Z

+
+    public addMember(member: PythonBase): void {
+        if (!(member instanceof StructField)) {
+            throw new Error('Must add TypedDictProperties to ValueType');


I am assuming this is StructField, no?

eladb · 2019-07-07T09:38:22Z

-        // and one with all optional keys in order to emulate this. So we'll go ahead
-        // and implement this "split" class logic.
+        // Not actually rendering these as class params,
+        // we splat the entire


"the entire..." what?

eladb · 2019-07-07T09:40:50Z

+        // Required properties, those will always be put into the dict
+        code.line('self._values = {');
+        for (const member of members.filter(m => !m.optional)) {
+            code.line(`    '${member.pythonName}': ${member.pythonName},`);


Do we need to validate that we actually got values in all required fields or are we leaving that to jsii?

Fair point. It's easy enough to add, might as well do it.

Ah no, we don't have to, Python will complain. The constructor looks like this:

def __init__(self, *, required, optional=None): ...

So you MUST pass a value for required.

eladb · 2019-07-07T09:46:59Z

-    bar: str
+@jsii.data_type(jsii_type="@scope/jsii-calc-base.BaseProps", jsii_struct_bases=[scope.jsii_calc_base_of_base.VeryBaseProps], name_mapping={'foo': 'foo', 'bar': 'bar'})
+class BaseProps:
+    def __init__(self, *, foo: scope.jsii_calc_base_of_base.Very, bar: str):


Why do we need the "*"?

It makes it so you can't pass any of the following values without using the keyword. Which is required, otherwise adding optional parameters later on might change the order which is breaking.

Oh wow. nice!

eladb · 2019-07-07T09:48:58Z

-    anotherOptional: typing.Mapping[str,scope.jsii_calc_lib.Value]
-    """This is optional.
+@jsii.data_type(jsii_type="jsii-calc.DerivedStruct", jsii_struct_bases=[scope.jsii_calc_lib.MyFirstStruct], name_mapping={'anumber': 'anumber', 'astring': 'astring', 'first_optional': 'firstOptional', 'another_required': 'anotherRequired', 'bool': 'bool', 'non_primitive': 'nonPrimitive', 'another_optional': 'anotherOptional', 'optional_any': 'optionalAny', 'optional_array': 'optionalArray'})
+class DerivedStruct:


Don't we want this to inherit from MyFirstStruct? Did we test struct inheritence?

Not even sure what that means.

Okay it will be used for nominal typing, probably.

eladb · 2019-07-07T09:50:37Z

    elif isinstance(obj, datetime.datetime):
        raise TypeError("Naive datetimes are not supported, please add a timzone.")
-    raise TypeError
+    raise TypeError("Don't know how to convert object to JSON: %r" % obj)


eladb · 2019-07-07T09:54:00Z

@garnaat please review

garnaat · 2019-07-07T12:35:56Z

LGTM

Add tests on generator struct equality members

48be6f6

rix0rrr added 2 commits July 5, 2019 14:36

Move struct -> jsii mapping deeper into kernel so it's more symmetric…

88b15a3

… with unmapping Remove now-unused dependency on mypy_extensions.

Updating build expectations

409bd73

garnaat reviewed Jul 5, 2019

View reviewed changes

Remove debug statement

9636f17

rix0rrr assigned eladb Jul 5, 2019

Generate ReST-style docstrings.

0a140b5

Don't need to emit serializer anymore

e7f3cef

rix0rrr added 3 commits July 7, 2019 09:32

Don't emit '*' if there are no kwargs in constructor

a17315a

Update expectations

80039a6

fix(python): respect mixed-case abbrevs while snake-casing (#588)

c84fc7d

Common mixed-case abbrevations will be snake-cased as a while, instead of being broken up into separate word parts.

eladb reviewed Jul 7, 2019

View reviewed changes

Add inheritance

1ad2c1a

eladb approved these changes Jul 7, 2019

View reviewed changes

eladb merged commit 51211a0 into master Jul 7, 2019

eladb deleted the huijbers/python-capitalization branch July 7, 2019 13:02

Conversation

rix0rrr commented Jul 5, 2019 • edited by eladb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rix0rrr commented Jul 5, 2019

Uh oh!

rix0rrr commented Jul 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr commented Jul 5, 2019

Uh oh!

garnaat commented Jul 5, 2019

Uh oh!

eladb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eladb commented Jul 7, 2019

Uh oh!

garnaat commented Jul 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rix0rrr commented Jul 5, 2019 •

edited by eladb

Loading