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

Propagation breaks down when provided times are not in UTC #9

Closed
parzivail opened this issue Aug 13, 2019 · 4 comments
Closed

Propagation breaks down when provided times are not in UTC #9

parzivail opened this issue Aug 13, 2019 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@parzivail
Copy link
Owner

Discovered in #8.

@bretcope
Copy link

bretcope commented Aug 13, 2019

From #8:

davidtgillard: Would the Kind of output DateTimes be kept consistent with input DateTimes? Or would the Kind of output DateTimes always be UTC?

I believe outputs should always be in UTC for consistency. The caller can always convert it to whatever timezone they'd like.

The documentation could indicate as such.

@parzivail
Copy link
Owner Author

That's reasonable. It would reduce overhead on our part and would leave the decision up to the consumer to convert to the time zone that they prefer, should they need something other than UTC.

@davidtgillard
Copy link
Contributor

davidtgillard commented Aug 13, 2019

On the face of it, this approach seems like surprising behavior to me - if a developer calls a pure function, e.g. Observe, with Datetimes in, say, NST, then it seems inconsistent that they would get back UTC DateTimes. However, the alternative with DateTime - trying to keep the DateTimes in a single well-defined and consistent zone - seems like a bad idea, because the API exposes time-related state for the caller to consume as they wish (e.g. EciCoordinate owns a DateTime). So I'd use DateTimeOffset to make it explicit in the datatype that it's all in UTC.

But the internet is strewn with DateTime vs DateTimeOffset discussions. Speaking practically, I'm happy with the proposed DateTime approach above.

@parzivail
Copy link
Owner Author

I created an extension method to safely convert to a UTC DateTime only when the Kind is known and checked all of the usages of DateTimes and created #10. Does this look viable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants