Fix nested dictionary preview #8734
Conversation
* update tests
|
@aparajit-pratap this seems good, is it worth it to add a test? I know the view tests are sometimes time consuming. @QilongTang @ramramps I believe this will break binary compatibility as a public method has changed its signature. I think we should definitely re-release the 2.0 nugets if this is merged. IMO: Nested dictionaries seem like a general enough problem that this should be merged. |
|
no nested dictionaries would be no good |
|
@mjkkirschner I can revert the public method signatures if needed as I've only changed the order of the argument (stuck it at the end) in order to make it a default argument. |
|
@QilongTang |
|
@ramramps This is unfortunate. Dynamo Nugets 2.0.0 are all out this morning since we need an official RC to test. Two options here in my mind:
|
|
@QilongTang Thanks. 2.0.0.xxxx after @aparajit-pratap changed the method signature? |
|
@aparajit-pratap LGTM 👍 nice tests. |
|
@Racel should this get cherry picked to RC 2.0? |
|
Plan A: merge to master, and we can test over the weekend and then merge to RC. Just in case QA pushes back. |
|
@aparajit-pratap @mjkkirschner @jnealb I am merging in now in case we need to cherry-pick this in the last minute. |
|
will test it when the build is posted |
Reverting public API signature changes made in #8734
|
@QilongTang @mjkkirschner If it has not been done yet, please cherry pick. This should be in 2.0 release. Regarding the nuget packages, can't you just "delete" the one we released with out this fix? |
|
@Racel There is not delete function there so it is not doable |
|
@QilongTang - "Hide" then? |
|
@Racel Even we hide it, we won't be able to upload 2.0.0 version again. It has to be a different version like 2.0.0.xxxx |
|
ah, ok. i think that is ok.... |
* update tests for List.Rank * Update failing tests (DynamoDS#8663) * update tests * fix nested dictionary preview * minor argument refactor * add test
* cherry-pick #8661 * update tests * Fix nested dictionary preview (#8734) * update tests for List.Rank * Update failing tests (#8663) * update tests * fix nested dictionary preview * minor argument refactor * add test * revert signatures of public APIs * change version number of test DYN file to 2.0.0
Purpose
This fixes QNTM-3947. Nested dictionaries will not have any information about preferred dictionary ordering. Therefore when the same ordering from the outer dictionary is passed into the nested dictionary preview, the ordering of the outer dictionary does not apply to the nested one resulting in an
key not found exception. In this case, we therefore pass null to the nested dictionary all the time. And we add a null check forpreferredDictinoaryorderingargument so that we check it only if a non-null argument for it is passed.Related Github issue: #8665
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@mjkkirschner
FYIs
@jnealb