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(rosetta): Rosetta is not submodule-aware #3176

Merged
merged 20 commits into from
Nov 18, 2021
Merged

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Nov 16, 2021

Rosetta was not properly aware of submodules and namespaces, at all. This manifested in two different ways:

  • Imports of (Python) modules from submodules were not translated properly
  • Synthetically generated struct constructors were not namespaced with a proper import.

The translation of a call involving a struct:

import { aws_ec2 as ec2 } from 'aws-cdk';

const construct = new ec2.MyConstruct('some', 'values', {
  arg: { ...  }
});

Used to be translated into this:

from aws_cdk import aws_ec2 as ec2

construct = ec2.MyConstruct('some', 'values',
    arg=DeeperStruct(...)
    )

Notice that DeeperStruct is missing the module prefix that would
allow this code to work.

We trace what jsii FQNs every imported symbol refers to, and see
if we can reuse an import that already exists to retrieve our struct.
If not, we'll generate a new import at the top of the example.

To properly track what is being imported in an example (if it's a class or
a module, or potentially a submodule) we needed to be able to identify
submodules in TypeScript source, so we've also attached a symbolId
to submodules in the jsii assembly.


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

Synthetically generated examples used to look like this:

```py
from aws_cdk import aws_ec2 as ec2

construct = new ec2.MyConstruct('some', 'values',
    arg=DeeperStruct(...)
    )
```

Notice that `DeeperStruct` is missing the module prefix that would
allow this code to work.

Trace what imports we've already done (to see if we can piggyback
onto any existing imports), otherwise generate new imports for the
namespaces we need to reference struct names from.
@rix0rrr rix0rrr requested a review from a team November 16, 2021 12:58
@rix0rrr rix0rrr self-assigned this Nov 16, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 16, 2021
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Nov 16, 2021

NOTE: not complete yet for submodules. Requires #3174 to finish.

@rix0rrr rix0rrr changed the title fix(rosetta): namespace inferred Python struct names fix(rosetta): Rosetta is not submodule-aware Nov 17, 2021
@rix0rrr rix0rrr requested review from RomainMuller and a team November 18, 2021 10:42
@mergify
Copy link
Contributor

mergify bot commented Nov 18, 2021

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 Nov 18, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 18, 2021

Merging (with squash)...

@mergify
Copy link
Contributor

mergify bot commented Nov 18, 2021

Merging (with squash)...

@mergify mergify bot merged commit 5c7d148 into main Nov 18, 2021
@mergify mergify bot deleted the huijbers/python-import-names branch November 18, 2021 18:05
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants