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

Include standard Duration pattern that matches the one used for JSON #1290

Closed
maciejjarzynski opened this issue Feb 22, 2019 · 3 comments

Comments

@maciejjarzynski
Copy link

commented Feb 22, 2019

The standard Duration pattern described in the docs:
https://nodatime.org/2.3.x/userguide/duration-patterns
is -D:hh:mm:ss.FFFFFFFFF
whereas the JsonConverter in
https://github.com/nodatime/nodatime.serialization/blob/edf88349e54ab63f06266a9c864edfa1ba6503cb/src/NodaTime.Serialization.JsonNet/NodaConverters.cs#L102

has a pattern -H:mm:ss.FFFFFFFFF.
Does this serve any purpose, or it can simply be DurationPattern.Roundtrip there?

@jskeet

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

I honestly don't remember the origin of using this instead of the standard pattern, but I'd argue that we shouldn't change it at this point; it would be a breaking change for stored information for little benefit.

Rather than change either, I think it would probably be more useful to add an extra standard pattern that corresponds with the JSON one. Does that sound like a reasonable compromise?

@jskeet jskeet self-assigned this Feb 22, 2019

@maciejjarzynski

This comment has been minimized.

Copy link
Author

commented Feb 22, 2019

Yes, for the sake of compatibility that would be an option, thanks ;)

@jskeet jskeet changed the title Default pattern for Duration JsonConverter is different from the standard one Include standard Duration pattern that matches the one used for JSON Feb 22, 2019

@jskeet

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Righto - renamed the issue for what it's turned into just for my convenience :)

@jskeet jskeet transferred this issue from nodatime/nodatime.serialization Feb 23, 2019

jskeet added a commit to jskeet/nodatime that referenced this issue Feb 23, 2019
Add a new roundtrip pattern for durations
This matches the pattern that NodaTime.Serialization.JsonNet uses,
which just has a number of hours (and smaller) rather than the days
and hours of the regular roundtrip pattern.

Fixes nodatime#1290.
jskeet added a commit to jskeet/nodatime that referenced this issue Feb 23, 2019
Add a new roundtrip pattern for durations
This matches the pattern that NodaTime.Serialization.JsonNet uses,
which just has a number of hours (and smaller) rather than the days
and hours of the regular roundtrip pattern.

Fixes nodatime#1290.
jskeet added a commit to jskeet/nodatime that referenced this issue Feb 23, 2019
Add a new roundtrip pattern for durations
This matches the pattern that NodaTime.Serialization.JsonNet uses,
which just has a number of hours (and smaller) rather than the days
and hours of the regular roundtrip pattern.

Fixes nodatime#1290.

@jskeet jskeet closed this in #1291 Feb 23, 2019

jskeet added a commit that referenced this issue Feb 23, 2019
Add a new roundtrip pattern for durations
This matches the pattern that NodaTime.Serialization.JsonNet uses,
which just has a number of hours (and smaller) rather than the days
and hours of the regular roundtrip pattern.

Fixes #1290.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.