eschutho commented on PR #44172: URL: https://github.com/apache/superset/pull/44172#issuecomment-5703820493
@rebenitez1802 thanks for the thorough pass — the High was a real gap (the mixin method I'd patched has no production caller). Pushed `c92265919732`: **High** — reverted `ExploreMixin.get_timestamp_expression` back to unguarded (it's dead code, `TableColumn` doesn't subclass `ExploreMixin`) and moved the two-clause handler into `TableColumn.get_timestamp_expression` instead, which is the method every call site actually dispatches to. Retargeted its two tests off the `__get__`-bound mixin onto the real method via the existing `_stored_col` fixture helper. **Medium** — added `SupersetTemplateException` to the caught tuple (with a `str(ex)` fallback) in the retargeted `get_timestamp_expression`, in `convert_tbl_column_to_sqla_col`, and in `get_rendered_sql` for consistency. **Low (2 of 4)** — extended the identical guard to the two sibling `get_sqla_col` methods (`TableColumn.get_sqla_col`, `SqlMetric.get_sqla_col`) since they're the same shape as what I was already touching. Fixed the tests injecting a raw `TemplateSyntaxError` (which `process_template` always re-wraps as `SupersetSyntaxErrorException` before a caller ever sees it) to inject the real wrapped type instead. Tightened `match=` assertions to the interpolated message, not just the static prefix. Restored the `else: # SupersetSyntaxErrorException` disambiguator. **Deliberately not done**, flagging rather than guessing at scope: the adhoc-SQL sibling paths you flagged (`_process_metric_sql_expression`/`_process_validated_sql_expression`) have zero existing exception handling and sit on a much broader, `noqa: C901`-complex path with more collaborators — wrapping those felt like a separate, riskier change rather than a same-shape extension, so leaving it as a follow-up rather than bundling it here. Also skipped the handler-dedupe suggestion given your own pybabel-extraction caveat. Full test file (92 tests) green, ruff/format clean, fail-before/pass-after verified individually for all 8 new/retargeted tests. Ready for another look. -- 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]
