Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: add validator package #322

Merged
merged 4 commits into from
Apr 1, 2020
Merged

feat: add validator package #322

merged 4 commits into from
Apr 1, 2020

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Mar 28, 2020

🏆 Enhancements

  • Move validators from incubator-superset for usage in packages
import { legacyValidateInteger, legacyValidateNumber, validateNonEmpty } from '@superset-ui/validator';

There is a non-legacy validateInteger and validateNumber 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

@kristw kristw requested a review from a team as a code owner March 28, 2020 01:41
@codecov
Copy link

codecov bot commented Mar 28, 2020

Codecov Report

Merging #322 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #322   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         103    108    +5     
  Lines        1230   1248   +18     
  Branches      303    310    +7     
=====================================
+ Hits         1230   1248   +18
Impacted Files Coverage Δ
.../superset-ui-validator/src/legacyValidateNumber.ts 100% <100%> (ø)
...kages/superset-ui-validator/src/validateInteger.ts 100% <100%> (ø)
...ckages/superset-ui-validator/src/validateNumber.ts 100% <100%> (ø)
...ages/superset-ui-validator/src/validateNonEmpty.ts 100% <100%> (ø)
...superset-ui-validator/src/legacyValidateInteger.ts 100% <100%> (ø)
test/setup.ts 100% <100%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98ed48b...18f4160. Read the comment docs.

@netlify
Copy link

netlify bot commented Mar 28, 2020

Deploy preview for superset-ui ready!

Built with commit 18f4160

https://deploy-preview-322--superset-ui.netlify.com

@@ -0,0 +1,8 @@
import { t } from '@superset-ui/translation';

export default function isNotEmpty(v: unknown) {
Copy link
Contributor

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?

Copy link
Contributor

@ktmud ktmud left a 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.

@kristw
Copy link
Contributor Author

kristw commented Mar 28, 2020

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

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome

@rusackas
Copy link
Member

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

@kristw kristw merged commit ea78a4e into master Apr 1, 2020
@delete-merged-branch delete-merged-branch bot deleted the kristw--validator branch April 1, 2020 00:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants