-
Notifications
You must be signed in to change notification settings - Fork 119
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 admonition about Date functions now using UTC #360
base: master
Are you sure you want to change the base?
Add admonition about Date functions now using UTC #360
Conversation
thadguidry
commented
Aug 5, 2024
- Fixes Fix date parsing regression introduced in 2018 OpenRefine#6009
✅ Deploy Preview for openrefine-website ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@tfmorris and @ostephens Do you think this would close that linked issue? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this! I was hoping someone was going to address documenting the incompatible changes.
I think the description needs fleshing out a little bit with a general discussion of the change and compatibility implications, as well as specific references near the affected functions (mostly toDate()
and toString()
, I think).
The current proposal only mentions benefits without any drawbacks. Is that intentional? It seems to me to leave out half the story.
@@ -447,6 +447,10 @@ As of OpenRefine 3.4.1, uniques() reorders the array items it returns; in 3.4 be | |||
|
|||
## Date functions {#date-functions} | |||
|
|||
:::info | |||
Date/times without timezone info were interpreted as **local** up until May 2018 when OpenRefine 3.0 was released, at which point they were switched from **local** to **UTC**. One benefit of this change was to introduce consistency and reproducibility when working collaboratively and sharing projects across timezones. Discussed in issue [#6009](https://github.com/OpenRefine/OpenRefine/issues/6009) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most relevant place for this information is with the toDate()
and toString()
documentation, although those should probably be brief references to a more general discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to have this generally at the beginning of discussing all the Date functions.
But would love your help on the toString()
documentation suggestions which I can add in this PR.
:::info | ||
Date/times without timezone info were interpreted as **local** up until May 2018 when OpenRefine 3.0 was released, at which point they were switched from **local** to **UTC**. One benefit of this change was to introduce consistency and reproducibility when working collaboratively and sharing projects across timezones. Discussed in issue [#6009](https://github.com/OpenRefine/OpenRefine/issues/6009) | ||
::: | ||
|
||
###### now() {#now} | ||
|
||
Returns the current time according to your system clock, in the [ISO 8601 extended format](exploring#data-types) (converted to UTC). For example, 10:53am (and 00 seconds) on November 26th 2020 in EST returns [date 2020-11-26T15:53:00Z]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR, but since it caught my eye, ISO 8601 isn't involved. Although this does the same thing as before, due to the changes in toString()
, the effective date/time is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand. This is the now()
function directly, which returns a date-time with an offset from UTC/Greenwich in the ISO-8601 calendar system, according to the Java docs. So I think this reads OK as is? But I think your point is that we need to improve the toString()
documentation more fully? I can do that in this PR also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, that's wrong... now()
will always return the date-time from the users system clock in the specified time-zone (which is Z
according to our code , which means Zulu time, which means we return the users system time without any offset.)
So should now()
docs stay as is, because it does return the users system clock in ISO 8601 format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only semantics, but the date handling is complicated enough without leading people astray. ISO 8601 dates are represented using strings. What the now()
function returns is an internal OpenRefine datetime object. The square brackets around "[date 2020-11-26T15:53:00Z]" and the leading "date" prefix indicate that this is the default toString()
rendering for an internal date object, not a string. The expression now().toString() would give "2020-11-26T15:53:00Z" for the above example. Using OpenRefine 2.8,
now()returns a date object rendered as "[date 2024-08-20T17:16:42Z]" while
now().toString()` returns "Aug 20, 2024" using my default system locale and now().toString("hh:mm a z") return "05:20 PM EDT" which matches the current local wall clock time.
In addition to date to string conversions, import/export from spreadsheet formats like Excel and OpenOffice behave differently as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, so you want to document that OpenRefine prior to 3.0 handled this differently? If that is your concern, I think it's less important how prior 3.0 handled things and just document how they currently work over the last few versions. Our surveys show that everyone is using at least OpenRefine 3.7+ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole point of #6009 is documenting (instead of fixing) the date handling regression. If that's not what this is addressing, you should remove the tag.
The discussion of now()
is separate. It doesn't return an ISO 8601 formatted string. It returns a datetime.
I'll let you and @ostephens decide how you're going to document this. My preference was to fix the bugs, but I got overruled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tfmorris Can you express how you would word things?
I think the description needs fleshing out a little bit with a general discussion of the change and compatibility implications, as well as specific references near the affected functions (mostly toDate() and toString(), I think).
Co-authored-by: Tom Morris <[email protected]>