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

fix(python): generate type-checking code #1881

Merged
merged 10 commits into from
Aug 21, 2020
Merged

Conversation

RomainMuller
Copy link
Contributor

There were a number of issues with the generated code that caused it not
to properly validate though mypy:

  • Properties and method names could collide with buit-in types
  • mypy is unable to check decorations on decorated declarations
    • Those must be opted out with #types: ignore
  • mypy is unable to check dynamic base classes
    • Those must be opted out with #types: ignore
  • Impossible protocol conformance through meta-class
    • Removed JSClass and Referenceable in favor of Type and Any
  • Incorrect inference for the _values dictionary in structs
    • Had to add proper type annotation there
  • Had to add some assert foo is not Null in the implementation of
    non-optional struct property getters, to satisfy type checking

Those issues were highlighted in awslabs/cdk8s#194


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

@RomainMuller RomainMuller added bug This issue is a bug. language/python Related to Python bindings effort/medium Medium work item – a couple days of effort module/pacmak Issues affecting the `jsii-pacmak` module contribution/core This is a PR that came from AWS. labels Aug 12, 2020
@RomainMuller RomainMuller requested a review from a team August 12, 2020 18:25
@RomainMuller RomainMuller self-assigned this Aug 12, 2020
There were a number of issues with the generated code that caused it not
to properly validate though `mypy`:
- Properties and method names could collide with buit-in types
- `mypy` is unable to check decorations on decorated declarations
  - Those must be opted out with `#types: ignore`
- `mypy` is unable to check dynamic base classes
  - Those must be opted out with `#types: ignore`
- Impossible protocol conformance through meta-class
  - Removed `JSClass` and `Referenceable` in favor of `Type` and `Any`
- Incorrect inference for the `_values` dictionary in *structs*
  - Had to add proper type annotation there
- Had to add some `assert foo is not Null` in the implementation of
  non-optional struct property getters, to satisfy type checking

Those issues were highlighted in awslabs/cdk8s#194
@RomainMuller RomainMuller force-pushed the rmuller/python-typing branch from 8983d7c to 46b448c Compare August 13, 2020 08:36
@mergify
Copy link
Contributor

mergify bot commented Aug 16, 2020

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Aug 16, 2020
@mergify
Copy link
Contributor

mergify bot commented Aug 16, 2020

Merging (with squash)...

@RomainMuller RomainMuller added this to the August Release milestone Aug 19, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
  • Commit ID: 75a4402
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@RomainMuller RomainMuller merged commit e6d1bc1 into master Aug 21, 2020
@RomainMuller RomainMuller deleted the rmuller/python-typing branch August 21, 2020 14:43
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/medium Medium work item – a couple days of effort language/python Related to Python bindings module/pacmak Issues affecting the `jsii-pacmak` module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants