Skip to content
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

Add mutation support #27

Closed
wants to merge 5 commits into from
Closed

Add mutation support #27

wants to merge 5 commits into from

Conversation

zoul
Copy link
Collaborator

@zoul zoul commented May 11, 2019

This adds mutation API described in #26. What remains to be done:

  • Add immutable update API such as updating(key:to:)
  • Update documentation

Also, in #26 I figured the best way to handle subscript-writing to a non-array value would be converting the value into an array. But then the index is out-of-bounds, so writing to a non-array simply changes the value to an empty array:

var json: JSON = true
json[1] = "foo" // now `json` is `[]`

This doesn’t look very helpful. Doing nothing is an option, but then we wouldn’t be consistent with writing to a key subscript. I’m not sure how to approach this. Could we change subscript writing for arrays and objects to do nothing if the underlying value is not an array or an object?

@zoul zoul requested review from cjmconie and ezamagni May 11, 2019 12:22
@zoul
Copy link
Collaborator Author

zoul commented May 15, 2019

I’m still thinking about making the subscript mutation do nothing for incompatible values:

var json: JSON = true
json[0] = 1
json.bar = "foo"
// json is still just `true`

The good thing about this would be having less silly corner cases. And if people want to force their way, they could always use merge or direct assignment instead.

@ezamagni ezamagni marked this pull request as ready for review May 19, 2019 12:59
GenericJSON/Merging.swift Outdated Show resolved Hide resolved
GenericJSON/Merging.swift Outdated Show resolved Hide resolved
// Writing to a non-array value converts the value into an empty array.
// TODO: This is not really very helpful, can we do better?
guard case .array(var arr) = self else {
self = .array([])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this would be a very strange behavior. It also doesn't match with the analogous subscript case for object values.
What i don't really like is that we are converting the value into an array, but we are leaving it empty, thus ignoring the new value completely.
What we could do is convert the value into an array that has all the values between 0 and index filled with null and the index-th set with new value.
Something like:

Suggested change
self = .array([])
self = .array(Array(repeating: JSON.null, count: index))
self[index] = newValue

However, for this behavior to be consistent we would have to modify line 30 as well and have a write to an out-of-bounds index behave as well.

Copy link
Collaborator Author

@zoul zoul Jun 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Growing the array automatically would be closer to JavaScript array semantics, but JavaScript uses some kind of sparse arrays and the “filler” is undefined, not null:

var a = []
a[10] = 1
a // [10: 1]
a[2] // undefined

So how about doing nothing both for string/int subscript writes if the value is not an object/array? As proposed above:

var json: JSON = true
json[0] = 1
json.bar = "foo"
// json is still just `true`

If people want to just overwrite the value no matter what, they can simply assign:

var json: JSON = true
json = [1]
json = ["bar": "foo"]

Or force-merge. This would work even if json is intially JSON.true, for example:

var json: JSON = ["foo": true]
json.merge(with: ["bar": "foo"])

We wouldn’t have any support for writing to arbitrary (out-of-bounds) array indices, but I doubt people really need that in Swift. What do you think?

@zoul zoul force-pushed the feature/mutation branch from 7e2976a to 9cac6c3 Compare July 4, 2019 11:30
@zoul zoul force-pushed the feature/mutation branch from 9cac6c3 to b566478 Compare July 4, 2019 11:40
@zoul
Copy link
Collaborator Author

zoul commented Nov 14, 2019

Sorry, I completely dropped this, no ETA in sight.

@zoul zoul closed this Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants