rusackas commented on PR #34513:
URL: https://github.com/apache/superset/pull/34513#issuecomment-3658304478
From the bot feedback:
1. Regex specificity - ❌ Not worth changing
The ^interval pattern is consistent with the entire codebase - there are
50+ similar patterns like ^tinyint, ^decimal, ^timestamp, etc. Changing just
this one would be inconsistent.
2. Type matching - ❌ Not an issue
The base class already uses .get() for lookup (see base.py:995), so this
works correctly in production.
3. KeyError in test - ❌ Minor style
Tests that fail with KeyError still clearly indicate the problem. Not
worth changing.
4. NULL handling - ⚠️ The only one worth considering
The current behavior (None→0) is fine for bar/pie charts since they need
numeric values. If you want to be more conservative, you could change it to
pass through None and let the chart layer decide. But this is a minor edge case
that probably doesn't warrant blocking the PR.
Happy to change my opinion on item 4 if anyone is worried about it.
--
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]
