-
Notifications
You must be signed in to change notification settings - Fork 273
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the function name be consistent with the file name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-superset
for usage in packagesThere is a non-legacy
validateInteger
andvalidateNumber
which 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