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

Accept ISO-8601 short time offset format #5015

Merged
merged 1 commit into from
Jul 24, 2020

Conversation

mtgto
Copy link
Contributor

@mtgto mtgto commented Jul 23, 2020

#1194 supports ISO8601 timestamp format.
lib/dates.js says it supports the ISO8601 time zone format.
In fact it supports the RFC3339 time offset format, not ISO8601.

time offset format of RFC3339 is:

time-numoffset  = ("+" / "-") time-hour ":" time-minute

https://tools.ietf.org/html/rfc3339#section-5.6

It always needs minutes.

In contract, ISO8601 time offset format can abbreviate minute if minute is 00.

The UTC offset is appended to the time in the same way that 'Z' was above,
in the form ±[hh]:[mm], ±[hh][mm], or ±[hh].

https://en.wikipedia.org/wiki/ISO_8601#Time_offsets_from_UTC


I noticed this problem while converting PostgreSQL response into chart.
Plotly.js does not allow the date and time output from PostgreSQL.

Example of datetime string from PostgreSQL: 1997-12-17 07:37:16-08 (minute of time offset is abbreviated)

The output format of the date/time types can be set to one of the four styles ISO 8601, SQL (Ingres), traditional POSTGRES (Unix date format), or German. The default is the ISO format.

https://www.postgresql.org/docs/12/datatype-datetime.html#DATATYPE-DATETIME-OUTPUT

@archmoj archmoj added status: reviewable community community contribution labels Jul 23, 2020
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 Thanks @mtgto - this looks like a useful addition, the tests are perfect. @archmoj unless you have any other comments feel free to merge when you're ready.

@archmoj archmoj added the bug something broken label Jul 24, 2020
@archmoj archmoj merged commit 556b5d4 into plotly:master Jul 24, 2020
@mtgto
Copy link
Contributor Author

mtgto commented Jul 24, 2020

Thanks for your review 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken community community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants