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

chore: clarify timezone requirement #150

Merged
merged 3 commits into from
Nov 30, 2022
Merged

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Oct 17, 2022

A small change to make the timezone info on the Datetime type optional. Some SDKs already do not include timezone info.

Since it's loosening this definition, this is non-breaking.

@agentgonzo
Copy link
Member

agentgonzo commented Oct 17, 2022

Which SDKs don't support timezones (and can they be taken out the back and shot?) ? Is it that they "don't" (and can thus be fixed to include them) or "can't" by virtue of the language.

I struggle to believe that they can't.

Ambiguous times are a terrible terrible thing to put in at the design stage. At the very least, the spec should explicitly state that if the timezone is not present, it WILL be treated at UTC but I would prefer to exhaust all other options before making timezone optional (unless it is something that is by definition zoneless like UNIX time).

@toddbaert
Copy link
Member Author

toddbaert commented Oct 17, 2022

Which SDKs don't support timezones (and can they be taken out the back and shot?) ? Is it that they "don't" (and can thus be fixed to include them) or "can't" by virtue of the language.

I struggle to believe that they can't.

Ambiguous times are a terrible terrible thing to put in at the design stage. At the very least, the spec should explicitly state that if the timezone is not present, it WILL be treated at UTC but I would prefer to exhaust all other options before making timezone optional (unless it is something that is by definition zoneless like UNIX time).

Maybe this is a phrasing issue. I was basically trying to address a possible misintrpetation of the previous requirement, namely: "you must set a tz/locale". I'm trying to refer to this change: open-feature/java-sdk#59

To your point in the PR above, timezone-less entities are better - it's optimal to make times "delocalized" but absolute, like Instant or unix-times; that's indeed what I'm trying to encourage. Maybe the original phrasing is better, and we can recommend a timestamp with an implicit or explicit UTC tz?

@agentgonzo
Copy link
Member

OK, it sounds like it's a phrasing issue and I'm overthinking it.
As long as the interpretation is "no timezone => UTC" then all is good.

specification/types.md Outdated Show resolved Hide resolved
@toddbaert toddbaert changed the title chore: make timezone on Datetime optional chore: clarify timezone requirement Oct 18, 2022
toddbaert and others added 2 commits November 9, 2022 15:27
Signed-off-by: Todd Baert <toddbaert@gmail.com>
Co-authored-by: Steve Arch <sarch@cloudbees.com>
Signed-off-by: Todd Baert <toddbaert@gmail.com>
@beeme1mr beeme1mr merged commit 5270ae8 into main Nov 30, 2022
@beeme1mr beeme1mr deleted the editorial/make-tz-optional branch November 30, 2022 20:08
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.

4 participants