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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add admonition about Date functions now using UTC #360

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1440a8e
Add admonition about Date functions now using UTC
thadguidry Aug 5, 2024
09cea0c
Update with review suggestion from tfmorris
thadguidry Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/manual/exploring.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ A “date” type is created when a column is [transformed into dates](transform

Date-formatted data in OpenRefine relies on a number of conversion tools and standards. For something to be considered a date in OpenRefine, it will be converted into the ISO-8601-compliant extended format with time in UTC: YYYY-MM-DDTHH:MM:SSZ.

:::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)
thadguidry marked this conversation as resolved.
Show resolved Hide resolved
:::

When you run <span class="menuItems">Edit cells</span> → <span class="menuItems">Common transforms</span> → <span class="menuItems">To date</span>, the following column of strings on the left will transform into the values on the right:

|Input|→|Output|
Expand Down
4 changes: 4 additions & 0 deletions docs/manual/grelfunctions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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.

Copy link
Member Author

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.

:::

###### 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].
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Member

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]" whilenow().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.

Copy link
Member Author

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+ ?

Copy link
Member

@tfmorris tfmorris Aug 21, 2024 •
edited
Loading

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.

Copy link
Member Author

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).

Expand Down