[WIP] Python JSII Support #219
Conversation
| // to do it in the code generation phase, because attempting to mix style and | ||
| // function makes the code generation harder to maintain and read, while doing | ||
| // this here is easy. | ||
| await shell("black", ["--py36", sourceDir], {}); |
There was a problem hiding this comment.
Do you plan to keep this for production or is this just for developing?
There was a problem hiding this comment.
My intent was to keep it for production, although I don't think it's a requirement if people feel strongly otherwise. One of my goals was that people could easily see the structure of an API by reading the generated code (obviously not the actual implementation) and formatting the code makes it much more pleasant to read, but emiting formatted code from the CodeMaker instance is annoying since you have to split logical lines over multiple lines, depending on the length of the line.
| this.buffer = []; | ||
| } | ||
|
|
||
| // We're purposely replicating the API of CodeMaker here, because CodeMaker cannot |
There was a problem hiding this comment.
Would it not make sense to push this change upstream into CodeMaker?
There was a problem hiding this comment.
It probably would yea, I duplicated it here primarily for expediency, and because I don't feel wholly comfortable submitting code to an OSS JS project since I'm not super familiar with the ecosystem. Although having duplicated it here will make some things easier in general (for instance, when I get support for typing Named types, being able to add imports for the named types, which we won't know what types to emit imports for until we've hit a onMethod or similar that has used them).
| protected onBeginClass(cls: spec.ClassType, abstract: boolean | undefined) { | ||
| debug("onBeginClass"); | ||
|
|
||
| // TODO: Figure out what to do with abstract here. |
There was a problem hiding this comment.
I think it might be sufficient to generate an __init__ method that throws?
There was a problem hiding this comment.
Which immediately makes it useless for anything other than type annotations. Speaking of, are you planning to include those?
There was a problem hiding this comment.
I left that as a note to myself because I wasn't sure what the semantics of an abstract class was supposed to be. Are people supposed to be able to subclass it? If so an ABC would probably be appropriate.
|
|
||
| # TODO: Replace with proper error. | ||
| assert ( | ||
| resp.hello == "[email protected]" |
There was a problem hiding this comment.
Ooh this hardcoded version number is scary to me. Are we going to have to remember to manually bump this on every release?
There was a problem hiding this comment.
My TODO should probably have been expanded to also include not hardcoding a version number here. This was one of the first bits of code I wrote for this, and I wasn't sure at the time what the versioning semantics were for the jsii-runtime. Ideally this will be a version range specifier (which will entail taking on more dependencies).
| # function. This is done to better mimic other decorators like @proeprty. | ||
| class jsii_property: | ||
|
|
||
| # We throw away the getter function here, because it doesn't really matter or |
There was a problem hiding this comment.
The alternative would have been to use the code generator to generate the effective code for every proxied call in the properties and methods--which seems more expected to me, honestly.
What is the advantage of putting the logic in the annotations?
There was a problem hiding this comment.
In general, I wanted to have the generated code be as simple as possible, and to be able to be used to follow the structure of the library (but obviously not the implementation). What you're suggestion would basically be, instead of:
class Foo(metaclass=JSIIMeta, jsii_type="something"):
@jsii_property
def my_property(self) -> str:
...To do something like:
class Foo(metaclass=JSIIMeta, jsii_type="something"):
@property
def my_property(self) -> str:
# Most direct translation, this could be made slightly nicer
# by import jsii.runtime.kernel and using that and removing
# the __jsii_kernel__ attribute.
return self.__jsii__kernel.get(self.__jsii_ref__, "my_property")Generally I just think the first one looks better. It uses ... to suggest that the implementation has been omitted (which it has, the implementation is off in TypeScript somewhere). It also means that bug fixes that don't affect the generated code can be made without requiring the entire JSII ecosystem to regenerate code, they can just pull in a new jsii-python-runtime (although that's probably unlikely, since the method call can be made to be roughly as simple as the property call itself).
Ultimately, it was primarily an aesthetics thing, and I vastly preferred the way generated code turned out when I was using decorators to implement the functionality. It lets the generated code sort of function more like a header file, and sort of matches (in spirit at least) the API of CFII where you embed the C header file in Python, and it creates an automagic Python object that has all of the C functions defined in that header on it. It's not a 1:1 mapping because the JSII is materially different, but it felt like the closest translation of that concept to me.
…ces = false" This reverts commit 6a2ec91.
WIP support for the Python support in the JSII.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.