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

Changed date serialization #1358

Merged
merged 7 commits into from
Jan 27, 2014
Merged

Changed date serialization #1358

merged 7 commits into from
Jan 27, 2014

Conversation

jchannon
Copy link
Member

@jchannon jchannon commented Dec 9, 2013

Fixes #1346

To revert to previous behaviour use the below code in the Bootstrapper:

JsonSettings.ISO8601DateFormat = false

@khellang
Copy link
Member

khellang commented Dec 9, 2013

Haha! You really should've kept the whitespace changes for a separate commit 😉

@jchannon
Copy link
Member Author

jchannon commented Dec 9, 2013

@khellang done 👅

@khellang
Copy link
Member

khellang commented Dec 9, 2013

Much better! 😄 Isn't the ISO 8601 format .ToString("u") or is that only if the time is UTC?

@jchannon
Copy link
Member Author

jchannon commented Dec 9, 2013

@phillip-haydon
Copy link
Member

Looks fine to me, need to document this as a breaking change though. Suspect anyone requiring the old shitty date formatting is going to have a cry if/when this is released.

@khellang
Copy link
Member

khellang commented Dec 9, 2013

You should be able to opt-out if your app breaks because of this...

@jchannon
Copy link
Member Author

jchannon commented Dec 9, 2013

I'm with Phillip. That's what release notes are for

@dot-i
Copy link
Contributor

dot-i commented Dec 9, 2013

Will this break all of us that rely on the WCF date format? If so, please do make this an optional thing. Or I will indeed cry loudly ;-)

If your app has a REST api based on Nancy -- that used to send out the WCF date format, all consumers will simply rely on this. You can't just go breaking this as all client apps will break horribly and will need to be rewritten...

@jchannon
Copy link
Member Author

jchannon commented Dec 9, 2013

Guess we could make it a setting in JsonSettings. Let's see what
@thecodejunkie & @grumpydev say

@dot-i
Copy link
Contributor

dot-i commented Dec 9, 2013

A JSON setting does sound like the right approach, yes 👍

@khellang
Copy link
Member

khellang commented Dec 9, 2013

The cost of adding an if-else statement to provide users with an opt-out solution is not really high. It's bad PR to tell the user that "we broke it and didn't provide a way to fix it again" in the release notes 😛

@prabirshrestha
Copy link
Contributor

@jchannon any reason why iso8601 isn't turned on by default?

@jchannon
Copy link
Member Author

Just to prevent people's stuff breaking when they upgrade

@phillip-haydon
Copy link
Member

@jchannon - we should push the new standard, but allow the user to revert back.

@khellang
Copy link
Member

we should push the new standard, but allow the user to revert back

Exactly, opt-out, not in 😉

@albertjan
Copy link
Contributor

yes!

@phillip-haydon
Copy link
Member

Basically @jchannon... You fucked up. Fix it. We still love you tho.

@grumpydev
Copy link
Member

Yeah, make the new behaviour default, mark it as a breaking change and put info in the issue text about how to change it back.

@grumpydev
Copy link
Member

Has anyone actually verified that this is the format that #1346 needs? :)

@khellang
Copy link
Member

.ToString("s") is shorthand for .ToString(DateTimeFormatInfo.SortableDateTimePattern) which is the format of ISO 8601 (local time). See MSDN docs.
You also have the DateTimeFormatInfo.UniversalSortableDateTimePattern which should be used if it's UTC, i.e. a DateTimeOffset. but I'm pretty sure that's a YAGNI for now 😉

@jchannon
Copy link
Member Author

@grumpydev you will have to label it as a breaking change as only you and Andreas can do that I believe

@prabirshrestha
Copy link
Contributor

+1 for default. better to break earlier than later.

thecodejunkie added a commit that referenced this pull request Jan 27, 2014
Changed date serialization
@thecodejunkie thecodejunkie merged commit 3502526 into NancyFx:master Jan 27, 2014
@jrsconfitto
Copy link
Contributor

🤘 This is the 💣

i've wanted this for a while. Thanks for doing this 😃

@jchannon
Copy link
Member Author

jchannon commented Feb 7, 2014

@jugglingnutcase no worries, thanks for the thanks 😄

@dot-i
Copy link
Contributor

dot-i commented Mar 9, 2014

A question about this change, regarding this line in Nancy.Json.JsonDeserializer:

StringBuilderExtensions.AppendCount(output, maxJsonLength, string.Concat("\"", value.ToString("s", CultureInfo.InvariantCulture), "\""));

Wouldn't it be better to change this to "o" (round-trip) format rather than "s"? You get more accuracy and also timezone information then. That way it's possible to differentiate between UTC and Local date-times.

Look at these examples in "o" format:

Output Description
2014-03-09T11:41:20.0719047+01:00 11:41 in my GMT+1 timezone
2014-03-09T11:41:20.0000000+00:00 11:41 in GMT timezone
2014-03-09T10:41:20.0719047Z 11:42 in UTC

In "s" format they are a bit too basic and cannot be told apart anymore:

Output Description
2014-03-09T11:41:20 will be parsed OK for my timezine
2014-03-09T11:41:20 will be parsed incorrectly, should have given 12:41 for my timezone
2014-03-09T10:41:20 will be parsed semi-incorrectly as it will appear as Local time

Any thoughts?

@dot-i
Copy link
Contributor

dot-i commented Mar 9, 2014

I actually went ahead and did the work to improve this (incl unit tests) and submitted this pull request: #1485 :-)

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

Successfully merging this pull request may close these issues.

Use ISO 8601 Dates in serializers
9 participants