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

Add [proposed] rfc3339 zoned date time #77

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sam-mfb
Copy link

@sam-mfb sam-mfb commented Sep 11, 2021

PR to address #76 by adding support for the proposed RFC 3339 extension to date time formats to allow serializing of time zone.

This would allow using a serialization format that is consistent with java and the pending Temporal proposal to ECMA/javascript

@jskeet
Copy link
Member

jskeet commented Sep 11, 2021

I wasn't expecting any special code for the RFC 3339 extension to be in the serializers code. I was expecting to write:

  • ZonedDateTimePattern.ExtendedRfc3339 (or something similar - I'm not hugely happy with that name) as a property
  • Just WithReplacementNodaTimeConverter changes in this repo

Does that make sense?

@sam-mfb
Copy link
Author

sam-mfb commented Sep 11, 2021

This isn't ready yet, but working on it highlighted a couple issues that I'm wondering how to deal with:

  1. On the converter itself, not sure how to handle the optional providing of calendar that the RFC3339 proposal would allow for. As I read that proposal, you can add [someCalendar] as a suffix, but you don't have to. The question then is (a) how do we want to deal with that here (ignore or try to incorporate) and (b) either way, how do we deal with the fact that it is optional. It doesn't look like there's a way to make the c option optional in the parser (at least not without a PR against nodatime itself)

  2. @jskeet had proposed an extension

public static JsonSerializer WithNodaTimePattern<T>(this JsonSerializer serializer, IPattern<T> pattern) 

I instead went for the accepting a fully formed converter over just the pattern. Thoughts on that?

@sam-mfb
Copy link
Author

sam-mfb commented Sep 11, 2021

sorry; you beat me to the punch; the reason i posted this as draft was to get a better understanding. so you'd prefer, to add a pattern to the main repo and the then the replacement converter here. that's fine. what do you think about the calendar issue and also the replacment converter vs replacment pattern issue?

@sam-mfb
Copy link
Author

sam-mfb commented Sep 11, 2021

and of course, any other comments :) no hurry; i just figured I'd bang something out while it was on my mind and i had a little time.

@jskeet
Copy link
Member

jskeet commented Sep 11, 2021

I'll try to have a closer look tomorrow for those questions.

@jskeet
Copy link
Member

jskeet commented Sep 15, 2021

In terms of pattern/converter: I think it might make sense to have both. It's slightly frustrating that we need the calendar validator so we can't really just have the one accepting a pattern, although I think that would be useful sometimes. But given that the converter code isn't specific to Noda Time, we could probably call that just WithReplacementConverter.

In terms of the calendar and other parameters: I'd suggest that our initial pattern, exposed as ZonedDateTimePattern.ExtendedRfc3339 or whatever, neither includes nor parses any parameters. We may want to create a pattern that supports the calendar parameter at some point, but that will require some more work in terms of checking exactly which calendar system is meant by each Unicode identifier.

@sam-mfb
Copy link
Author

sam-mfb commented Sep 15, 2021

Thanks for the guidance, Jon. This makes sense. On the calendar parameter, if I understand, that means the pattern will fail if passed 2020-05-22T07:19:35.356-04:00[America/Indiana/Indianapolis][u-ca=islamic-umalqura] even though technically that's a compliant string under the rfc3339 proposal. That makes sense to me as the best behavior since we aren't supporting the calendar parameter, i.e., better to fail loudly than silently. Assuming we can't come up with a better name than ExtendedRfc3339 (I haven't been able to think of one...), should it be ExtendedRfc3339WithoutCalendar to reflect we aren't handling that part of the spec?

@jskeet
Copy link
Member

jskeet commented Sep 15, 2021

I would suggest something like "plain" rather than "without calendar" - it won't support any parameters, whether they're [u-ca=...] or anything else.

@sam-mfb
Copy link
Author

sam-mfb commented Sep 15, 2021

It does seem like at some point, if the RFC3339 extension is approved as written, you'll want an optional calendar pattern, e.g. C. I say that because it looks the spec is contemplating that both 2020-05-22T07:19:35.356-04:00[America/Indiana/Indianapolis][u-ca=islamic-umalqura] and 2020-05-22T07:19:35.356-04:00[America/Indiana/Indianapolis]. And so I assume those should ultimately be handled by a single converter (which would therefore need an optional parser). That also raises the question about how to serialize--i.e., do you always include the calendar suffix? only when it's not ISO? So, yeah, I guess the calendar part will be tricky.

@jskeet
Copy link
Member

jskeet commented Sep 15, 2021

My guess is that in reality, the number of people who use it with a calendar parameter will be small. We'll see :)

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.

2 participants