Skip to content

JSONEncoder/JSONDecoder and String convenience methods #144

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

Merged

Conversation

fabianfett
Copy link
Member

Motivation:

Little common denominator after a long discussion in #129: JSONEncoder and JSONDecoder should have convenience methods to encode into String and decode from String.

Result:

  • It's a little easier to deal with json payloads that are delivered in a String. (APIGateway, SNS, SQS)
  • Public API is increasing

@fabianfett fabianfett force-pushed the ff-jsonencoder-convenience branch from 1975c85 to 417ec9f Compare July 23, 2020 14:44

extension JSONEncoder {
/// Convenience method to allow encoding json directly into a String. It can be used to encode a payload into an APIGateway.V2.Response's body.
public func encodeAsString<T: Encodable>(_ value: T) throws -> String {

Choose a reason for hiding this comment

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

can be refactored as
public func encodeAsString<Element: Encodable>(_ value: Element) throws -> String


extension JSONDecoder {
/// Convenience method to allow decoding json directly from a String. It can be used to decode a payload from an APIGateway.V2.Request's body.
public func decode<T: Decodable>(_ type: T.Type, from string: String) throws -> T {

Choose a reason for hiding this comment

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

can be refactored as
public func decode<Element: Decodable>(_ type: Element.Type, from string: String) throws -> Element

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Mixed feelings about this one to be honest. One one hand that's harmless okey API and a nice quality of life change, but on the other adding much public convenience APIs on Foundation's coders is somewhat meh... I.e. not every library using JSONEncoders should invent many convenience APIs to them as we could end up with similar or same APIs defined in many libs etc...

Anyway, not a strong opinion

@ktoso
Copy link
Contributor

ktoso commented Jul 27, 2020

I somewhat fail to see how this specific extensions are addressing the wider goal that was discussed in the linked issue #129 @fabianfett ? Tomer's comment #129 (comment) outlines a somewhat wider effort than just the string coding functions -- i.e. by adding functions to the gateway handler, rather than just to specific JSON coder types.

What am I missing here?

@ktoso
Copy link
Contributor

ktoso commented Jul 27, 2020

Chatted some more; as I mentioned, not really strong opinion here so going to leave to @tomerd to decide if we're ok with these 👍

@tomerd
Copy link
Contributor

tomerd commented Jul 31, 2020

my only concern here is with extending public types from the SDK from which may conflict with other libraries if they do the same - we typically try to avoid doing so

@weissi wdyt?

@fabianfett
Copy link
Member Author

my only concern here is with extending public types from the SDK from which may conflict with other libraries if they do the same - we typically try to avoid doing so

I don't want to steer the discussion in one direction and I'm sure you're aware, but let me mention for completeness, that we also extend the JSONEncoder and JSONDecoder for ByteBuffer.

@fabianfett fabianfett force-pushed the ff-jsonencoder-convenience branch from 09545e3 to 9d28bfe Compare August 3, 2020 18:46
@fabianfett fabianfett merged commit 2067f4a into swift-server:master Aug 3, 2020
@fabianfett fabianfett deleted the ff-jsonencoder-convenience branch August 3, 2020 19:04
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.

4 participants