Conversation
Codecov Report
@@ Coverage Diff @@
## master #322 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 103 108 +5
Lines 1230 1248 +18
Branches 303 310 +7
=====================================
+ Hits 1230 1248 +18
Continue to review full report at Codecov.
|
|
Deploy preview for superset-ui ready! Built with commit 18f4160 |
| @@ -0,0 +1,8 @@ | |||
| import { t } from '@superset-ui/translation'; | |||
|
|
|||
| export default function isNotEmpty(v: unknown) { | |||
There was a problem hiding this comment.
should the function name be consistent with the file name?
ktmud
left a comment
There was a problem hiding this comment.
Can you give an example of how these validators are used in Superset? I'm also not sure about coupling translations with the validators. Most patterns I saw put the validation function and error messages in separate config fields.
They are from https://github.com/apache/incubator-superset/blob/master/superset-frontend/src/explore/validators.js and used in https://github.com/apache/incubator-superset/blob/master/superset-frontend/src/explore/controls.jsx and the control panel config files in https://github.com/apache/incubator-superset/tree/master/superset-frontend/src/explore/controlPanels I am also not sure why they were designed this way. Trying to do minimum work to faciliate @rusackas and @villebro 's work as mentioned in one of the migration PR |
It is a little funny to couple translation (of error messages) with the validators. There are probably a few ways to address this, but for now it at least centralizes the burden of providing the string to the UI (i.e. it's DRY, at least). It's not something I'm particularly worried about, but I'm making a laundry list of tickets, and will put it on that list for discussion of how/when to remove this reliance. If you have the bandwidth to tackle it, that's fantastic too :D |

🏆 Enhancements
incubator-supersetfor usage in packagesThere is a non-legacy
validateIntegerandvalidateNumberwhich has the logical behavior, but will break superset because it seems to expect the wrong output from the legacy functions at the moment.@rusackas @villebro