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): incorrect transliteration of map keys in python #3449

Merged
merged 3 commits into from
Mar 29, 2022

Conversation

RomainMuller
Copy link
Contributor

The Python transliterator failed to properly account for maps when
transliterating keys, resulting in name-mangling being applied on these
when it should not have been.

This PR adds a new inMap context entry to track whether a property
assignment must be interpreted as a map key-value entry, or an actual
keyword/value mapping; and in that case circumvents the mangling.

Added a new test to verify that such map literals render appropriately
using a key similar to that which caused #3448.

Fixes #3448


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

The Python transliterator failed to properly account for maps when
transliterating keys, resulting in name-mangling being applied on these
when it should not have been.

This PR adds a new `inMap` context entry to track whether a property
assignment must be interpreted as a map key-value entry, or an actual
keyword/value mapping; and in that case circumvents the mangling.

Added a new test to verify that such map literals render appropriately
using a key similar to that which caused #3448.

Fixes #3448
@RomainMuller RomainMuller requested a review from a team March 29, 2022 18:47
@RomainMuller RomainMuller self-assigned this Mar 29, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 29, 2022
@@ -608,19 +622,18 @@ export class PythonVisitor extends DefaultVisitor<PythonLanguageContext> {
node: ts.StringLiteral | ts.NoSubstitutionTemplateLiteral,
_context: PythonVisitorContext,
): OTree {
const rawText = node.text;
if (rawText.includes('\n')) {
if (node.getText(node.getSourceFile()).includes('\n')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats this difference between this and node.text?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node.text is the JavaScript value of the string (in particular, with escape sequences replaced with their literal value, e.g: \n will be a "real" newline), whereas node.getText() will return the corresponding source code (which will have the escape sequences untouched).

The change here improves the situation a bit (avoid multi-lining literals that were not actually multi-line in the source), but it's also not perfect (probably good enough?) as it'll still cause some escapes to be replaced within multi-line literals).

@mergify
Copy link
Contributor

mergify bot commented Mar 29, 2022

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 Mar 29, 2022
@mergify
Copy link
Contributor

mergify bot commented Mar 29, 2022

Merging (with squash)...

@mergify mergify bot merged commit 54cebaa into main Mar 29, 2022
@mergify mergify bot deleted the rmuller/rosetta-py-map branch March 29, 2022 20:27
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Mar 29, 2022
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.

CORS Error despite thoroughly reading CDK docs, CDK docs error in the public reference
2 participants