Repository navigation
chore(python): use builtins.str instead of just str in Dict keys - #3866
Merged
Merged
Conversation
Added an integration test to verify parameters named `builtins` or `str` do not shadow the relevant type names, which used to be problematic until aws#3848 fixed the overarching issue, hoever the existing test only covered the case where a parameter name shadowed an namespace/import. Additionally, changes various `typing.Dict` keys that were mistakenly spelled out as `str` to use `builtins.str` in order to improve overall consistency in type declarations (see also aws#3866). Co-authored-by: rmuller@amazon.fr
RomainMuller
changed the title
RomainMuller
approved these changes
Dec 1, 2022
Contributor
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
Contributor
|
Merging (with squash)... |
DanielMSchmidt
added a commit
to hashicorp/terraform-cdk
that referenced
this pull request
Dec 1, 2022
The issue causing this problem will be released in the next days, but we have other problems in the release pipeline that should be fixed beforehand. Therefore I'll disable these tests for now and re-add them when updating JSII. The fix we are waiting for to be released is aws/jsii#3866
DanielMSchmidt
added a commit
to hashicorp/terraform-cdk
that referenced
this pull request
Dec 1, 2022
The issue causing this problem will be released in the next days, but we have other problems in the release pipeline that should be fixed beforehand. Therefore I'll disable these tests for now and re-add them when updating JSII. The fix we are waiting for to be released is aws/jsii#3866
jsteinich
added a commit
to open-constructs/cdk-terrain
that referenced
this pull request
Sep 12, 2026
### Related issue Follow-up from the review on #400, point 2. #400 is closed; this is the durable half of its regression coverage. ### Description `test/python/edge/test.ts` has been `describe.skip`'d since 3acb935 (**1 December 2022**), waiting on [aws/jsii#3866](aws/jsii#3866), with the stated intent to "re-add them when updating JSII". That fix shipped shortly after, and the JSII update has since landed (#395: jsii 6.0, jsii-pacmak 1.140.0) — but the skip was never lifted. Python is the only language whose edge test does not run. TypeScript, Go, Java and C# all do. That gap has a concrete cost: it is why #311's cross-language compile coverage never saw the broken Python provider import, which only surfaced once CI could reach Terraform >= 1.8 (#398) — and then via the `python-documentation` example rather than a test aimed at it. `main.py` calls `edge.provider.EdgeProvider(...)`, so `beforeAll`'s synth imports the generated `edge.provider` module — exactly the module whose `__init__.py` carried the unresolvable import. A regression there fails the suite loudly, at the import. ### Two stale assertions, and why they were wrong Re-enabling produced **18 passed / 2 failed**. The suite is substantially intact after three years dormant; both failures were expectations that had never once executed. **1. A typo.** The `numList` expectation read `...reqnum)}` — a stray closing paren — where synth produces `...reqnum}`. **2. A wrong assumption about `Fn.lookup`.** The map expectations assumed every `Fn.lookup(map, key, default)` renders as a Terraform `lookup()`. It does not. `Fn.lookup` (`packages/cdktn/src/terraform-functions.ts:41`) emits `lookup()` only when the default is **truthy**: ```ts static lookup(inputMap: any, key: string, defaultValue?: any) { if (defaultValue) return Fn._lookup(inputMap, key, [defaultValue]); return asAny(propertyAccess(inputMap, [key])); // -> renders inputMap[key] } ``` The fixture passes the same `Fn.lookup(...)` call for all six attributes, with different defaults: | attribute | default | truthy | renders as | | --- | --- | --- | --- | | `reqMap` | `false` | no | `${map_resource.map.reqMap.key1}` | | `optMap` | `"missing"` | yes | `${lookup(map_resource.map.optMap, "key1", "missing")}` | | `computedMap` | `0` | no | `${map_resource.map.computedMap.key1}` | So the deciding factor is the truthiness of the default, **not** whether the attribute is required, optional or computed. The corrected expectations match, and are now identical to `test/typescript/edge/test.ts`. Only the first failing assertion in each block is reported by jest, so the rest of the map block was corrected at the same time rather than surfacing them one CI run at a time. ### These pin current behaviour, not necessarily correct behaviour (#416) Worth flagging rather than burying: that truthiness guard looks like a bug. `Fn.lookup(map, "key", false)` reads as "return `false` if the key is missing", but the falsy default is discarded and the emitted expression becomes a bare property access — which errors on a missing key instead of returning the default. The guard should almost certainly be `defaultValue !== undefined`. Filed as #416. I have deliberately **not** changed it here. This PR is test coverage; altering `Fn.lookup` is a behavioural change to the construct library with its own blast radius, and it would be wrong to smuggle it in. If it is fixed, these expectations move with it — and at that point the edge tests will be the thing that proves the fix, which is rather the point of un-skipping them. Note this also means the equivalent TypeScript assertions were changed to match this behaviour in 09ac17b ("chore: fix assertions", Aug 2023) with no rationale recorded. ### Testing CI on this branch: **257 pass, 0 fail**, 1 skip (`windows_integration`, `if: false`). Both `python/edge` jobs pass, on Terraform 1.5.7 and 1.16.1. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added an integration test to verify parameters named
builtinsorstrdo not shadow the relevant type names, which used to be problematic
until #3848 fixed the overarching issue, hoever the existing test only
covered the case where a parameter name shadowed an namespace/import.
Additionally, changes various
typing.Dictkeys that were mistakenlyspelled out as
strto usebuiltins.strin order to improve overallconsistency in type declarations (see also #3866).
Co-authored-by: rmuller@amazon.fr
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.