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

Represent JSON as string even if Swift can't do it #610

Merged
merged 3 commits into from Nov 17, 2016

Conversation

gsabran
Copy link
Contributor

@gsabran gsabran commented Aug 31, 2016

Swift cannot represent dictionaries with nil values as json (http://stackoverflow.com/questions/13908094/how-to-include-null-value-in-the-json-via-nsjsonserialization) and the current SwiftyJSON string representation that relies on the default swift JSON string representation will fail as way.

Here I created a custom string representation that doesn't doesn't rely on the built in one, and can handle things with nil or values. I also added a few tests.

@zhigang1992
Copy link
Contributor

hm, not sure if I follows.

Is this what you're looking for?

screen shot 2016-09-22 at 12 41 55 am

Unless your intention is to convert Optional.None into null.
If that's the case, IMHO, I think they are a bit different, like Optional.None aka nil means this thing doesn't exist.
Where as null mean this thing (or key) exist, but the value is null.

So if the api is asking you to send null for empty fields, maybe we can map nil it to NSNull.

But doing it in the framework level seems a bit to specific.

What do you think? 😄

@gsabran
Copy link
Contributor Author

gsabran commented Sep 21, 2016

Yes, I'm trying to address the conversion of nil to json. Many objects have optionals and it felt natural to be able to convert them to json, and those json to string representation. It's really weird to think that a json with nil should have no string representation.
Currently it's not possible to convert such a json object to a string representation.

To you, how does the value is null differ from this thing doesn't exists. In both cases, there's no confusion on wether or not the key exists, because both

["foo": nil] as [String: Any?]

and

["foo": NSNull()] as [String: Any]

convey the idea that there's a key foo. I guess this question is not really json related, but more like what difference in meaning do you see between the two objects above?

@zhigang1992
Copy link
Contributor

zhigang1992 commented Sep 21, 2016

Thanks for the quick reply,

In you example, type [String: Any?] is not quiet the same as [String: Any].

IMHO, JSON dictionary is a type [String: JSON] or in this case [String: Any].

Optional type is swift only. You can't find 1 to 1 mapping in JSON that represent Optional.

Then with your example.

var json:[String: Any] = ["hello":"world"]
setting something to nil.
json["sup"] = nil remove the sup key if exist, and does nothing if it doesn't.
setting it to NSNull
json["sup"] = NSNull() would actually create a key with sup and set its value to NSNull.

Hope I'm making sense here.

😄

@gsabran
Copy link
Contributor Author

gsabran commented Sep 22, 2016

I agree this is all very confusing. I've dig a bit into this, and found even more confusing things.

I get your point that json["sup"] = nil deletes one key instead of setting a value to nil, and I believe that this shorthand is some bad design from swift more than anything else, especially given the questions raised in the SO post linked above. I'd much rather do json.removeValue(forKey: "sup") but this is a personal opinion.

I also think we need to have something that fits the daily challenges of developers, so I don't think it's fine to not be able to serialize dictionaries with nil values, that for instance could represent object with optionals fields. So if the solution is to drop the fields with nil values and serialized what's remaining, this is already much better than saying we can't convert json to string.

NSNull and NSJSONSerialization are Objective C things, and their design probably make sense there since there is no optional (other than that, I don't know Objective C that well).
But frankly in Swift I don't get the point of NSNull and the only time I've considered using it was to replace nil in dictionaries to parse them to json. So this might also be a bit of my personal opinion, but to me Swift NSNull should be forgotten, Swift nil is equivalent to json null, and Swift should forget about its shorthand to remove keys from dictionaries.

Happy to learn/discuss anything I'm missing here!

@gsabran
Copy link
Contributor Author

gsabran commented Sep 26, 2016

Is the discussion closed? What about having this as an optional way to cast to string? Like

func rawString(
  _ encoding: String.Encoding = .utf8,
  options opt: [JSONSerialization.WritingOptions],
  maxObjectDepth: Int = 10
) throws -> String? {
  // then, if an option is passed to cast `nil` to `NSNull`, do it, else don't, like
  // rawString(options: [.prettyPrinted, .castingNilToNSNull])
  ...
}

// and keep the original one for backward compatibility:
func rawString(
  _ encoding: String.Encoding = .utf8,
  options opt: JSONSerialization.WritingOptions = .prettyPrinted
) -> String? {
  return rawString(encoding, options: [opt])
}

@zhigang1992
Copy link
Contributor

sorry, I didnt close this on purpose, we cleaning the swift3 branch since we no long need it.

@zhigang1992
Copy link
Contributor

Can you try pointing this to master branch?

@gsabran gsabran changed the base branch from swift3 to master September 26, 2016 15:35
@zhigang1992
Copy link
Contributor

I'd definitely prefer to having a separate method or option doing this.

Not sure if you can extend JSONSerialization.WritingOptions though.

@gsabran
Copy link
Contributor Author

gsabran commented Sep 26, 2016

Ok I've rebased

@gsabran
Copy link
Contributor Author

gsabran commented Sep 26, 2016

Let me see what I can do for the option

@gsabran
Copy link
Contributor Author

gsabran commented Sep 26, 2016

I've changed things so that:

  • the previous syntax works as before
  • you have more options to describe the string casting, like json.rawString(options: [.castNilToNSNull: true])

@zhigang1992 zhigang1992 added this to the 3.1.2 milestone Oct 7, 2016
@zhigang1992 zhigang1992 modified the milestones: 3.2, 3.1.2 Nov 16, 2016
@zhigang1992
Copy link
Contributor

hey mate, sorry for the late reply.

Try a couple times solve the conflict and merge this PR, but seems like I'm can't push to you repo.

kylefang:SwiftyJSON/ (PR-610) $ gp gsabran PR-610:swift3                                                                                                                [0:32:59]
Counting objects: 299, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (294/294), done.
Writing objects: 100% (299/299), 57.89 KiB | 0 bytes/s, done.
Total 299 (delta 174), reused 0 (delta 0)
remote: Resolving deltas: 100% (174/174), completed with 14 local objects.
To https://github.com/gsabran/SwiftyJSON.git
 ! [remote rejected] PR-610 -> swift3 (permission denied)
error: failed to push some refs to 'https://github.com/gsabran/SwiftyJSON.git'

Can you help, will merge this ASAP.

@gsabran
Copy link
Contributor Author

gsabran commented Nov 16, 2016

Ah sure. Let me look at what has change on the main branch

@gsabran
Copy link
Contributor Author

gsabran commented Nov 16, 2016

I've rebased

@zhigang1992 zhigang1992 merged commit da5ed7f into SwiftyJSON:master Nov 17, 2016
@zhigang1992
Copy link
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants