sadpandajoe commented on code in PR #45410:
URL: https://github.com/apache/superset/pull/45410#discussion_r4363709709
##########
superset/charts/client_processing.py:
##########
@@ -835,6 +863,51 @@ def union_currency_context(
}
+def is_strftime_expressible(d3_time_format: str) -> bool:
+ """Whether ``strftime`` renders a d3 time format the way the browser
does."""
+ if LOCAL_TIME_PREFIX in d3_time_format:
+ # ``TimeFormatterRegistry.get`` strips the marker before building the
+ # formatter, so leaving it in would render it as literal text.
+ return False
+ directives = re.findall(r"%(.)", d3_time_format)
+ if not directives:
+ # Nothing for ``strftime`` to substitute, so it hands the string back
+ # unchanged and every cell would read the format itself. This is also
+ # what rejects the aliases the time format controls offer, which the
+ # browser resolves to a formatter that picks a representation per value
+ # rather than to a format string: ``smart_date`` ("Adaptive
+ # formatting", ``SMART_DATE_ID`` in ``superset-ui-core/time-format``)
+ # among them.
+ return False
+ if "%" in re.sub(r"%.", "", d3_time_format):
+ # A ``%`` no directive consumed, as the trailing one in ``%Y%``. d3
+ # drops it (``2021``) and ``strftime`` copies it (``2021%``), so the
+ # two disagree even where every directive is shared. ``%%`` is not
+ # this case: both read it as one escaped ``%``.
+ return False
+ return all(directive in SHARED_TIME_DIRECTIVES for directive in directives)
+
+
+def format_time_column(df: pd.DataFrame, column: Any, d3_time_format: str) ->
None:
+ """Format a temporal column in place with an explicit d3 time format.
+
+ The caller gates this on ``is_strftime_expressible``, so every directive in
+ ``d3_time_format`` means the same thing here as it does in the browser.
Null
+ values are left as-is rather than rendered as the string ``"NaT"``.
+
+ An aware value is read in UTC. ``createD3TimeFormatter`` builds the
+ formatter with ``utcFormat`` unless the format asks for local time, which
+ ``is_strftime_expressible`` rejects, whereas ``strftime`` would use the
+ value's own offset and put a different instant in the report.
+ """
+ values = df[column]
+ if isinstance(values.dtype, pd.DatetimeTZDtype):
+ values = values.dt.tz_convert("UTC")
+ df[column] = values.apply(
+ lambda value: value.strftime(d3_time_format) if pd.notna(value) else
value
Review Comment:
Passing the format through unchanged leaves surrounding whitespace in the
cell: the browser's formatter registry trims the format before building the
formatter, so `d3TimeFormat: " %Y "` shows `2024` in the chart but `" 2024 "`
in the report. Would it make sense to `.strip()` the format before validating
and formatting it, as the registry does?
##########
superset/charts/client_processing.py:
##########
@@ -835,6 +863,51 @@ def union_currency_context(
}
+def is_strftime_expressible(d3_time_format: str) -> bool:
+ """Whether ``strftime`` renders a d3 time format the way the browser
does."""
+ if LOCAL_TIME_PREFIX in d3_time_format:
+ # ``TimeFormatterRegistry.get`` strips the marker before building the
+ # formatter, so leaving it in would render it as literal text.
+ return False
+ directives = re.findall(r"%(.)", d3_time_format)
+ if not directives:
+ # Nothing for ``strftime`` to substitute, so it hands the string back
+ # unchanged and every cell would read the format itself. This is also
+ # what rejects the aliases the time format controls offer, which the
+ # browser resolves to a formatter that picks a representation per value
+ # rather than to a format string: ``smart_date`` ("Adaptive
+ # formatting", ``SMART_DATE_ID`` in ``superset-ui-core/time-format``)
+ # among them.
+ return False
+ if "%" in re.sub(r"%.", "", d3_time_format):
+ # A ``%`` no directive consumed, as the trailing one in ``%Y%``. d3
+ # drops it (``2021``) and ``strftime`` copies it (``2021%``), so the
+ # two disagree even where every directive is shared. ``%%`` is not
+ # this case: both read it as one escaped ``%``.
+ return False
+ return all(directive in SHARED_TIME_DIRECTIVES for directive in directives)
+
+
+def format_time_column(df: pd.DataFrame, column: Any, d3_time_format: str) ->
None:
+ """Format a temporal column in place with an explicit d3 time format.
+
+ The caller gates this on ``is_strftime_expressible``, so every directive in
+ ``d3_time_format`` means the same thing here as it does in the browser.
Null
+ values are left as-is rather than rendered as the string ``"NaT"``.
Review Comment:
The docstring says a null would be rendered as the string `"NaT"`, but
`pd.NaT.strftime(...)` raises `ValueError: NaTType does not support strftime`,
so without the `pd.notna` guard a single null timestamp fails the whole report
rather than printing text. Could the comment describe that instead? (The same
wording appears in `test_table_time_format_keeps_nulls`.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
