[SIP-15A] Enforcing ISO 8601 date/timestamp formats #7702
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7702 +/- ##
==========================================
+ Coverage 67.65% 67.65% +<.01%
==========================================
Files 448 448
Lines 22498 22501 +3
Branches 2364 2364
==========================================
+ Hits 15220 15223 +3
Misses 7140 7140
Partials 138 138
Continue to review full report at Codecov.
|
john-bodley
force-pushed
the
john-bodley--iso-8601
branch
from
70491c0
to
2906158
Compare
| String or Integer(epoch) type`)} | ||
| {t(' expression which needs to adhere to the ')} | ||
| <a href="https://en.wikipedia.org/wiki/ISO_8601"> | ||
| {t('ISO 8601')} |
There was a problem hiding this comment.
none of this i18n stuff really works, but since it was bad before I don't see it worthwhile to fix now (or how best to fix it while still embedding links). Maybe we should add a TODO here to do the i18n the right way in the future?
| @@ -99,14 +99,23 @@ function ColumnCollectionTable({ | |||
| label={t('Datetime Format')} | |||
| descr={ | |||
| <div> | |||
| {t('The pattern of the timestamp format, use ')} | |||
| {t('The pattern of timestamp format. For string use ')} | |||
john-bodley
force-pushed
the
john-bodley--iso-8601
branch
4 times, most recently
from
fe4a0f0
to
2e53eb1
Compare
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue |
john-bodley
force-pushed
the
john-bodley--iso-8601
branch
2 times, most recently
from
c30af86
to
62d6a1b
Compare
john-bodley
changed the title
| "python_date_format": [ | ||
| # Restrict viable values to epoch_s, epoch_ms, or a strftime format | ||
| # which adhere's to the ISO 8601 format (without time zone). | ||
| Regexp( |
There was a problem hiding this comment.
@dpgaspar this FAB validator works but it doesn't seem to show the message. I've successfully used validators in Superset on other PRs and I was wondering whether there's a FAB issue displaying messages when the form field isn't on the primary tab.
john-bodley
force-pushed
the
john-bodley--iso-8601
branch
from
62d6a1b
to
3438a06
Compare
mistercrunch
added
🏷️ bot
CATEGORY
Choose one
SUMMARY
Per SIP-15A this PR helps to ensure that the when specifying a string like temporal column the date/timestamp
strftimeformat adheres to the ISO 8601 format. The TL;DR is if we're going to filter (and thus order) dates we need to use a representation where the lexicographical order coincides with the chronological order (see the ISO 8601 section in the SIP for details).This PR updates the CRUD and inline datasource editor (and adds validation where appropriate). Regrettable it seems in FAB we can't couple multiple fields, i.e., if the "Is temporal" field is unchecked then disable the "Datetime Format" field (@dpgaspar may know of how to do this). Additionally the messaging seems to be omitted if the form field is invalid (I was able to make this work for the default "Detail" tab (per here) but not for the "Columns" tab.
I originally when down the rabbit hole of replacing the
strftimeformatting to using the more restrictive ISO 8601 constructs, i.e.,YYYY-MM-DD hh:mm:ss.SSSSSSincluding providing a migration. I finally opted against this as thedatetimeobject doesn't support formatting using the ISO 8601 tokens (packages likearrowandpendulumndo however) and it seems like we heavily depend of thestrftimeformat for transforming Pandas dataframe.Note in the future I think there may be merit in trying to migrate to using the ISO 8601 tokens as this seems more standard and a time format which a number of engines support.
Finally there is no migration or corrective actions for currently ill-defined formats. This PR simply ensures that new formats are correct moving forward.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
CI and tested the regex validator.
ADDITIONAL INFORMATION
REVIEWERS
to: @betodealmeida @michellethomas @mistercrunch @villebro