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 ISO-8601 representation of intervals #270

Closed
GoogleCodeExporter opened this issue Mar 15, 2015 · 7 comments
Closed

Support ISO-8601 representation of intervals #270

GoogleCodeExporter opened this issue Mar 15, 2015 · 7 comments
Labels
Milestone

Comments

@GoogleCodeExporter
Copy link

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

For some reason our JSON representation of an Interval is something like this:

    { Start: "2013-07-26T16:45:20.1234567Z",
      End: "2013-07-26T17:45:20.1234567Z" } 

When it could be just a single value:

   "2013-07-26T16:45:20.1234567Z/2013-07-26T17:45:20.1234567Z"

That's a natural extension of ISO-8601 to cover subseconds.

This should probably be our ToString representation too (which is currently 
explicitly not defined).

Original issue reported on code.google.com by jonathan.skeet on 21 Feb 2014 at 9:22

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

Would it be OK to reuse the Patterns and Parsers for Instant directly in the 
Interval pattern/parser classes (both JSON and ToString())? Or does this go 
against how the code should be structured? 

I currently don't see this reuse happening elsewhere, but I believe that to be 
because reuse doesn't make sense in the other classes (and not because it is a 
design choice). Could the Interval parser use the instant parser? Or should 
this classes be separated?

For example is something like this ok:

private static class Patterns
{
    internal static readonly IntervalPattern ExtendedIsoPatternImpl = CreateWithInvariantCulture(string.Format("{0}'/'{0}", InstantPattern.ExtendedIsoPattern));
    internal static readonly IntervalPattern GeneralPatternImpl = CreateWithInvariantCulture(string.Format("{0}'/'{0}", InstantPattern.GeneralPattern));
}

Original comment by smea...@yahoo.com on 21 Feb 2014 at 4:53

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

I suspect it would be feasible, but for the moment I don't think I'll introduce 
an IntervalPattern. Instead, I'd *just* handle parsing for JSON.NET, and 
formatting is pretty simple to do within ToString itself. Will look over the 
weekend.

Original comment by jonathan.skeet on 21 Feb 2014 at 5:12

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

That format may be valid ISO-8601, but it's not very good JSON.  Part of the 
advantage to having them split as separate properties is that you can address 
each part separately (which is essential in performing range queries, for 
example).

I think it is appropriate to use the ISO format when calling 
Interval.ToString(), but I would leave it as is for JSON.

Another way to think about it is that each instant is sortable when considered 
separately, but not when combined into a single string.

Original comment by mj1856 on 22 Feb 2014 at 11:05

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

Well we *have* to keep the existing behaviour as the default anyway, for 
backwards compatibility purposes. This would just be an extra option.

Note that the result would still be sorted, but by start then end.

The point about being able to address each part separately is a very good one 
though, and certainly enough justification to keep the existing format as well 
(and probably keep it as the default) forever. Having an alternative 
single-string form for situations where that's more convenient seems reasonable 
though.

Original comment by jonathan.skeet on 22 Feb 2014 at 11:09

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

Sure, as long as it's configurable, I don't see a problem with that.

Original comment by mj1856 on 23 Feb 2014 at 1:52

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

Implemented in revision 9533b7548fe3 (Interval.ToString) and revision 
2d284594f750 (Json.NET).

I'm *slightly* uncomfortable about the Json.NET implementation as it means 
removing other converters first. An alternative would be to insert the new 
converter as the head of the list.

Original comment by jonathan.skeet on 23 Feb 2014 at 9:42

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 15, 2015

Original comment by malcolm.rowe on 30 May 2014 at 8:32

  • Changed state: Fixed
@malcolmr malcolmr modified the milestone: 1.3.0 Mar 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.