Skip to content
This repository has been archived by the owner on Mar 23, 2021. It is now read-only.

Offer a String API #55

Open
remko opened this issue Sep 29, 2016 · 7 comments
Open

Offer a String API #55

remko opened this issue Sep 29, 2016 · 7 comments

Comments

@remko
Copy link
Contributor

remko commented Sep 29, 2016

Am I correct that the only JSON serialization format supported right now is raw data? I'm guessing this is because this is what JSONSerialization offers?

In practice, I find it annoying to have to work with raw data, as I always work with Strings when using JSON, and let later stages care about which encoding to put it on the wire. As far as I understand, this means I have to always run this through String:validatingUTF8 (and know that the raw data Jay returns is UTF-8, which is undocumented at the moment), and force-unpack the optional (which I can be 100% sure is valid utf-8 anyway); vice versa, it means I have to call .utf8 every time I want to parse something with Jay (which is slightly less annoying than the former).

Wouldn't it be good to offer an API that parses and serializes to Strings?

Also, the fact that the API only takes raw bytes means you have to document which encoding it is in and which encoding it outputs. JSONSerialization takes any encoding, Jay only seems to take UTF-8, (see #54 ).

@czechboy0
Copy link
Collaborator

Actually, Jay should have public extensions to convert between [UInt8] and String, so it is just one method call to convert between them.

The new api would just do this conversion for you, nothing else. With the knowledge of the convenience conversion methods I mentioned above, would you still find an extra API so useful? It'd have to come in at least 4 variants, so it'd require additional tests and maintenance. But I don't feel strongly about it either way.

@remko
Copy link
Contributor Author

remko commented Sep 29, 2016

I should probably have not filed 2 requests, because I'm probably giving everyone more work with my duplicate posts :)

As I said in #54 , I would personally even drop the [UInt8] API if you only take UTF-8 strings anyway, because it leaves room for errors on the user side. Only having a String API will make sure that, if all the user has is raw data, they at least know they have to think about which encoding it is in when the compiler tells them Jay needs Strings.

@czechboy0
Copy link
Collaborator

There's a huge performance issue if we'd force people to convert to string when receiving data from the network when they want JSON. So I don't think we'll remove it.

@remko
Copy link
Contributor Author

remko commented Sep 29, 2016

Actually, Jay should have public extensions to convert between [UInt8] and String, so it is just one method call to convert between them.

Such an API would mean you either:

  • Have to return an optional string, which is silly, because all Jay produces is valid UTF-8
  • Live with the fact that your function can be abused and throw runtime errors, because the user could pass in any [UInt8] technically. You'd have to document that "You can only pass in data generated from Jay", but it's a shame in a typed language to have to document and put implicit contracts on APIs, if you could have avoided it with a built-in String API.

@remko
Copy link
Contributor Author

remko commented Sep 29, 2016

There's a huge performance issue if we'd force people to convert to string when receiving data from the network when they want JSON. So I don't think we'll remove it.

Fair enough, there will be a hit in this case. However, I'd argue that passing data directly from the wire to JSON is not a common case, so I'd leave this as an "advanced, you know what you're doing" API, and offer something less dangerous and more convenient for the common case.

I haven't looked at the API, but doubling it for just parse() and serialize() doesn't sound that bad, and you could probably even share all the test code?

@czechboy0
Copy link
Collaborator

Yup, it's a reasonable request. But re your first point - Jay is used by Vapor, the leading swift Web framework, and going between network data and parsed JSON is extremely common there, so performance is definitely key here. A better error and more documentation should hopefully help here (and possibly add that String API later).

@remko
Copy link
Contributor Author

remko commented Sep 29, 2016

You're right; it's not because many users won't use this API directly, that they won't use it a lot indirectly through lower dependencies such as Vapor.

Would you take a PR for a stringFrom or *FromString variant for methods that take [UInt8]? They would just be String() wrappers for the [UInt8] methods, so they would only need a single test each I would say.

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

No branches or pull requests

2 participants