-
Notifications
You must be signed in to change notification settings - Fork 191
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(sdk): improve delete() method for MutJson class #6278
Changes from all commits
a1f7d56
fbacc74
6b75dd0
7654108
654cedb
746a865
5865ebb
2d1ebb4
bf4d56d
ffd3bed
5e3b69b
27ae23e
bcfd6e2
4f5a0b4
215ae00
41c369f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -186,4 +186,26 @@ test "delete() for MutJson" { | |
assert(mutObj.has("x") == false); | ||
assert(mutObj.has("y")==true); | ||
assert(mutObj.delete("random key that doesn't exist") == true); | ||
let nullObj = MutJson{}; | ||
assert(nullObj.delete("something") == true); | ||
let mutJsonArray = MutJson [1, 2, 3]; | ||
assert(mutJsonArray.delete(1) == true); | ||
let boolMutJson = MutJson[true, 1, 2, 3]; | ||
assert(boolMutJson.delete(true) == true); | ||
assert(boolMutJson.has(true) == false); | ||
assert(boolMutJson.has(1) == true); | ||
let original = MutJson ({ | ||
"string": "wing", | ||
"number": 123, | ||
"array": [1, 2, 3], | ||
"true": true, | ||
"false": false, | ||
"object": { | ||
"key1": "value1", | ||
"key2": 2, | ||
"key3": false, | ||
"key5": [3, 2, 1] | ||
} | ||
}); | ||
assert(original.delete("key5")==true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This returning true feels off. Since "key5" is not a top level key of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see why it feels off, technically the returning true makes sense since "key5" was never present as a top level key of |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense for Json Arrays, since their order is preserved but perhaps in these cases its better to just convert types