Page MenuHomePhabricator

reimplementing all prior validators

Authored by leoeer on Jun 26 2020, 7:17 PM.



this diff is for duplicating all functionality currently present in dagster_pandas validators, albeit more efficiently and with more metadata.

Test Plan

Unit, adding more tests

Diff Detail

R1 dagster
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

leoeer created this revision.Jun 26 2020, 7:17 PM
leoeer requested review of this revision.Jun 26 2020, 7:31 PM
leoeer updated this revision to Diff 17672.Jun 30 2020, 4:14 PM

tests/samples for new system

leoeer updated this revision to Diff 17759.Jul 1 2020, 2:53 PM

multivalidation test

leoeer updated this revision to Diff 17763.Jul 1 2020, 3:16 PM

isort and black

leoeer updated this revision to Diff 17765.Jul 1 2020, 3:20 PM

isort and black

max added a comment.Jul 1 2020, 3:59 PM

I'd like to see unit tests of the validators (this will also make their semantics clear); for the unit tests of the solids, prefer execute_solid rather than the full execute_pipeline machinery. I want to encourage you to be more pedantic in docstrings and names -- it can be hard to figure out exactly what everything is doing at first glance, and you can assume with general validity that users have no time or patience. For the public APIs, I'd like to see example usage in docstrings.


don't be afraid to repeat yourself in docstrings. these will typically not be consumed together but in isolation.




what's the advantage of constructing metadict rather than passing these explicitly as kwargs? (the disadvantage is it obscures the contract of the underlying ConstraintWithMetadataException)


do we really need to support both forms -- could we instead settle on and enforce the dict, etc


why do we need a special affordance for this


i'm not really seeing the point of this sugar


shouldn't we throw/fail validation?


does this operate on a single value or on a column?


is this decorator intended to be public or is it purely internal?

max requested changes to this revision.Jul 1 2020, 4:00 PM
This revision now requires changes to proceed.Jul 1 2020, 4:00 PM
leoeer updated this revision to Diff 17775.Jul 1 2020, 4:44 PM

several changes to docs and tests

leoeer updated this revision to Diff 17781.Jul 1 2020, 5:22 PM

fixing docstrings breaking tests

leoeer updated this revision to Diff 18672.Mon, Jul 13, 10:47 PM

update deprecation language

max accepted this revision.Tue, Jul 14, 1:01 PM

this does what it says, but we could stand to invest a little more in explaining how. from my perspective, the docstrings are the most important thing here because they are how users will decide whether to try to use this machinery and whether it can solve their problems.


good to note the default here


does this run the risk of collision




no TODOs in docstrings

This revision is now accepted and ready to land.Tue, Jul 14, 1:01 PM
leoeer updated this revision to Diff 18700.Tue, Jul 14, 1:59 PM

massive docs rewrite

leoeer updated this revision to Diff 18701.Tue, Jul 14, 2:00 PM

todo out of docstring

Harbormaster failed remote builds in B15276: Diff 18701!
leoeer updated this revision to Diff 18705.Tue, Jul 14, 2:32 PM

datetime range support