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

Support Date marshalling with different formatters #73

Open
hartbit opened this issue Nov 3, 2016 · 9 comments
Open

Support Date marshalling with different formatters #73

hartbit opened this issue Nov 3, 2016 · 9 comments

Comments

@hartbit
Copy link

hartbit commented Nov 3, 2016

I'm currently using Unbox but am seriously thinking of switching to Marshal. But it has one major problem for me. The way that Date gets marshalling support by conforming to a protocol means I can't have dates parsed with different formatters. This is a real problem for applications that needs to parse JSON from different APIs that represent dates differently.

Any ideas of improving this?

@jarsen
Copy link
Member

jarsen commented Nov 3, 2016

Good question. What sort of formats are you getting?

I think there are a couple ways you might be able to deal with this:

let dateString: String = json.value(for: "someDate")
let date = myDateFormatter.date(from: dateString)
  1. Use marshal to extract to a String (like above), and then use whatever date formatter you desire
  2. Provide some way to inject a dateFormatter: maybe use some sort of shared instance. Then you can change the formatter to the desired formatter before extracting dates.
  3. Have an array of date formatters that you cycle through in your implementation until it finds one that parses correctly.

There are probably other ways to go about it, but that's what came to me off the top of my head. I'm sorry there's not something built in for this—dates are complicated... which is why we left them as an exercise for the user. We used to have dates built in but removed them for that reason.

Definitely open to suggestions though, if you have any ideas I'd love to hear them.

@jarsen jarsen added the question label Nov 3, 2016
@hartbit
Copy link
Author

hartbit commented Nov 4, 2016

Here are my quick impressions:

  1. It's an OK solution, but its less elegant than having the library support Date natively (which is very common ;-))
  2. If I understand correctly, the solution is somewhere there.
  3. That sounds very bad:
    • Bad for performance: Date parsing is often the most performance sensitive part of the decoding process.
    • Bad for correctness: Its not because a date formatter can parse a date that its the correct date (Day-Month-Year vs Month-Day-Year).

@bwhiteley
Copy link
Contributor

What about something like this?:
public func value<A: ValueType, B>(for key: KeyType, transform:(A) throws -> B) throws -> B
let date:Date? = try object.value(for: "date", transform: myDateFormatter.date)

@jarsen
Copy link
Member

jarsen commented Nov 4, 2016

Oh yes, 1 was not intended to be elegant and 3 was not supposed to be performant :)

I'm not as concerned by providing everything that other JSON decoders provide as I am doing the right thing for Marshal.

Changing the API as Bart suggests is a much easier/better way to inject a transformer into the decoding process and is something I've thought about—there's just a balance between adding features to the framework to handle different cases and keeping the framework small, which is why we removed the original implementation of date handling.

I don't think dates are the only thing that would ever potentially need some sort of transformer to pass along—so potentially adding a new protocol would be appropriate... but that's API growth.

I am far from opposed to adding new API, but we need to be critical about it or Marshal will grow beyond a comfortable size.

@jarsen
Copy link
Member

jarsen commented Nov 4, 2016

And there's not really a lot of documentation about this but Marshal's intent is not to solely to be a JSON decoding framework, but more general than that—which is why the MarshaledObject protocol exists. You could make all sorts of things conform to MarshaledObject and then initialize object from them. So while handling dates is often important in JSON, and we have special functions for JSON parsing (an example of framework growth), we need to be careful to think broadly about the API changes we affect.

@jarsen
Copy link
Member

jarsen commented Nov 4, 2016

All this said, I would love to see a proposal with a new protocol with a function like @bwhiteley suggested, and maybe then we can discuss how the API changes more concretely.

@jarsen
Copy link
Member

jarsen commented Nov 4, 2016

Since we're talking about Unbox, I think Unbox does a good job of keeping this general with its UnboxFormatter protocol. I'd like to be even more general (at least in naming, functionally they would be equivalent) and avoid the name Formatter so that we can keep the concept more flexible. That's one reason I really like Bart's proposal because it just takes a transformation function, rather than some sort of "Formatter" object.

@KingOfBrian
Copy link
Contributor

What about making Date conform to UnmarshalingWithContext and have the context be the date formatter? I believe this resolves any issues here and doesn't expand the API space

@jarsen
Copy link
Member

jarsen commented Dec 14, 2016

@KingOfBrian I think that could be a good solution, although perhaps not ideal semantically. I'd be ok having some additional API if it's not a huge addition and it adds clarity/value.

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

No branches or pull requests

4 participants