fix(sdk): improve delete() method for MutJson class #6278
Conversation
Signed-off-by: monada-bot[bot] <monabot@monada.co>
Signed-off-by: monada-bot[bot] <monabot@monada.co>
Signed-off-by: monada-bot[bot] <monabot@monada.co>
Signed-off-by: monada-bot[bot] <monabot@monada.co>
Signed-off-by: monada-bot[bot] <monabot@monada.co>
|
Thanks for opening this pull request! 🎉
|
|
Intended for having delete() work for non-map MutJson instances as brought up by @Chriscbr and @MarkMcCulloh from #6236 |
Signed-off-by: monada-bot[bot] <monabot@monada.co>
monadabot
added
the
⚠️ pr/review-mutation
|
|
||
| The key. | ||
| Either the key or index to delete. |
There was a problem hiding this comment.
Im not sure we should allow deletion at an index. While it may seem like order is preserved in most JSON objects, this is not a guarantee, according to the json specification "An object is an unordered set of name/value pairs"
There was a problem hiding this comment.
It makes sense for Json Arrays, since their order is preserved but perhaps in these cases its better to just convert types
| "key5": [3, 2, 1] | ||
| } | ||
| }); | ||
| assert(original.delete("key5")==true); |
There was a problem hiding this comment.
This returning true feels off. Since "key5" is not a top level key of original it should be original["object"].delete("key5")
There was a problem hiding this comment.
I can see why it feels off, technically the returning true makes sense since "key5" was never present as a top level key of original, but original["object"].delete("key5") should also return true as well with it actually deleting the present key
|
@partha04patel do you need any additional help here? seems like we are almost done! |
Checklist
pr/e2e-fulllabel if this feature requires end-to-end testingBy submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.