SEPURI-SAI-KRISHNA commented on code in PR #43225:
URL: https://github.com/apache/superset/pull/43225#discussion_r3998655225
##########
superset/charts/schemas.py:
##########
@@ -568,17 +570,18 @@ class
ChartDataRollingOptionsSchema(ChartDataPostProcessingOperationOptionsSchem
Rolling operation config.
"""
- columns = (
- fields.Dict(
- metadata={
- "description": "columns on which to perform rolling, mapping
source "
- "column to target column. For instance, `{'y': 'y'}` will
replace the "
- "column `y` with the rolling value in `y`, while `{'y': 'y2'}`
will add " # noqa: E501
- "a column `y2` based on rolling values calculated from `y`,
leaving the " # noqa: E501
- "original column `y` unchanged.",
- "example": {"weekly_rolling_sales": "sales"},
- },
- ),
+ columns = fields.Dict(
Review Comment:
Fair concern, and I would rather not fix it by widening this PR.
The frontend change is #43589, and it is already approved and mergeable --
it retypes
`columns` from `string[]` to `Record<string, string>` in
`superset-ui-core/src/query/types/PostProcessing.ts` for the rolling, diff
and cum
types. So the ordering you are asking for is available without any new work:
merge
#43589 first, then this one, and the two contracts never disagree in a
released state.
For what it is worth the window is narrower than it looks. Master publishes
no
`columns` property on `ChartDataRollingOptionsSchema` at all today -- the
trailing
comma dropped the field before apispec ever saw it -- so the spec is
currently silent
rather than wrong. Merging this first would move it from silent to
contradicting the
exported type, which I agree is worse than silence, hence the ordering.
Happy to hold this PR until #43589 lands if that is easiest for you. I would
rather not
pull the TypeScript change in here, since it would put a frontend edit in a
PR that is
otherwise Python-only and would make it harder to revert either half
independently.
--
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]
