rusackas commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4889056439
Thanks for this @abderbejaoui, the validation and the -1 bypass handling
look solid. I cleared out a pile of bot threads that were either stale or
over-applying the type-hint rule to test locals.
Two things I think are worth addressing before we merge:
The big one is the empty-input behavior in the reducer. When you clear one
of the cache-timeout fields back to an empty string, `Number('')` is 0, not
NaN, so `schema_cache_timeout`/`table_cache_timeout` end up stored as `0` and
`cache_timeout` as `'0'`. That's "never expire" rather than unset/global
default, so there's no way to clear a value back to the default once it's set.
I think clearing should drop the key / go to undefined. The bot flagged this
three times (schema/table + chart) and I think it's the same real issue in all
of them.
Also the new `cache_timeout` branch in `dbReducer` isn't covered by a test,
the frontend tests exercise the `ExtraOptions` inputs but not the reducer
itself. A small test pinning the clear-to-empty behavior would let us merge
with more confidence, especially since that's where the fix above lands. Thanks!
--
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]
