fix(python): duplicated kwargs when field is multi-inherited #2654
Conversation
When a struct field is inherited from more than one parent (this could be twice from the same type - in the case of a diamond shape; or from two distinct parents that declare the same field), the lifted keyword arguments in python would be duplicated for this field. This is because the method that performs the keyword argument lifting did not perform name-based de-duplication, and operates directly on the "raw" assembly (whereby it must traverse the inheritance tree itself, as opposed to the Go generator which uses `jsii-reflect` and has the field collection done by that library). Added the necessary de-duplication logic and confirmed the produced code now looks correct using the exact same test harness as I had introduced in #2650. Fixes #2653
| const knownProps = new Set<string>(); | ||
| for ( | ||
| let current = stack.shift(); | ||
| current != null; |
There was a problem hiding this comment.
Why change from undefined to null. Per docs, this will return undefined when empty?
Maybe just check for truthiness instead?
There was a problem hiding this comment.
x != null is the same as x !== nul && x !== undefined, but more compact.
And I don't like truthiness checks, because this is ripe for bugs.
packages/jsii-pacmak/package.json
Outdated
| @@ -64,6 +64,7 @@ | |||
| "@types/yargs": "^16.0.0", | |||
| "eslint": "^7.21.0", | |||
| "jest": "^26.6.3", | |||
| "jsii": "^0.0.0", | |||
There was a problem hiding this comment.
Intentional? pacmak depends on jsii?
Not familiar with the jsii layering but smells fishy.
There was a problem hiding this comment.
New tests actually use jsii to compile some tiny projects to validate code-gen output. This is very practical.
| @@ -0,0 +1,1437 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
There was a problem hiding this comment.
Does this require another new snapshot test?
Did this not manifest in the previous snapshot test for diamond dependencies - https://github.com/aws/jsii/pull/2591/files#diff-305aec203005012473bdfe3d0c5d9c734c7ee318a0e813759c82aed7067323e3 ?
Snapshot tests make it really hard to evaluate if a problem was actually fixed. It's useful to detect unintended regressions but perhaps it shouldn't be the sole testing mechanism.
There was a problem hiding this comment.
The alternative is to build (I mean compile) the generated code, for possibly many small projects, to ensure they "work" -- this is bound to be very slow/expensive. Also - next up is "but the fact the code compiles is not enough - we have to run it, too", and then it becomes a full time job?
|
The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged. |
# Conflicts: # packages/jsii-pacmak/test/generated-code/__snapshots__/examples.test.ts.snap # packages/jsii-pacmak/test/generated-code/examples.test.ts
RomainMuller
force-pushed
the
rmuller/diamond-python
branch
from
7a11eb7 to
08ccae9
Compare
RomainMuller
removed
the
pr/do-not-merge
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
|
Merging (with squash)... |
When a struct field is inherited from more than one parent (this could
be twice from the same type - in the case of a diamond shape; or from
two distinct parents that declare the same field), the lifted keyword
arguments in python would be duplicated for this field.
This is because the method that performs the keyword argument lifting
did not perform name-based de-duplication, and operates directly on the
"raw" assembly (whereby it must traverse the inheritance tree itself,
as opposed to the Go generator which uses
jsii-reflectand has thefield collection done by that library).
Added the necessary de-duplication logic and confirmed the produced code
now looks correct using the exact same test harness as I had introduced
in #2650.
Fixes #2653
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.