SEPURI-SAI-KRISHNA commented on PR #43710:
URL: https://github.com/apache/superset/pull/43710#issuecomment-5596371739
Thanks for the rebase and for catching the second drift, the
`ChartDataExtrasSchema`
enum needed to go for exactly the reason you gave, and I would not have
spotted it from
my side of the conflict.
Worth restating what this PR still does, because #43585 landed the same core
narrowing
while this was open and the title now overstates the change.
**Already fixed by #43585.** The Prophet field validating against
`PROPHET_TIME_GRAIN_MAP` rather than `get_time_grain_choices()` is master's
behaviour.
My `schemas.py` change is reduced to routing that same set through a named
`get_prophet_time_grain_choices()` helper, readability, not behaviour. If
you would
rather not carry the extra indirection, dropping that helper and keeping the
tests
would be a reasonable trim; I have no attachment to it.
**Still outstanding before this PR.** #43585 changed the validator but did
not
regenerate the spec, so master publishes a Prophet `time_grain` enum that is
narrower
than what the code accepts:
| | Prophet `time_grain` enum |
|---|---|
| master's `openapi.json` | 19 entries |
| master's `PROPHET_TIME_GRAIN_MAP` | 21 keys |
| this branch | 21 entries |
The two missing from the published spec are `PT0.5H` and `P0.25Y`. Both load
fine
through the schema on master; they are simply undocumented. That is the `+2`
in
`openapi.json` here, and together with your extras resync it puts the spec
back in
agreement with the code.
**Test coverage.** Three tests, none of which have an equivalent in master:
- `test_prophet_schema_advertises_only_resolvable_grains`, asserts the
advertised set
equals the set `prophet()` can map, so the spec and the operation cannot
drift apart
again silently. This is the one that would have caught the gap above.
- `test_prophet_accepts_every_mapped_grain`, parametrised over the map, so a
grain
added to `PROPHET_TIME_GRAIN_MAP` without a schema update fails here.
- `test_prophet_choices_exclude_an_addon_the_shared_helper_includes`, pins
the
intended asymmetry: a `TIME_GRAIN_ADDONS` key stays valid for the shared
helper and
stays out of Prophet's set.
So: a spec resync, a regression guard against the drift that just happened,
and an
optional helper. Happy to retitle it along those lines if you would prefer
the history
to read accurately, or to trim it to just the spec and tests, your call.
--
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]
