Page MenuHomePhabricator

reimplementing all prior validators
ClosedPublic

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

Details

Summary

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

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
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.

python_modules/libraries/dagster-pandas/dagster_pandas/constraints.py
386

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

421

yikes

425

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)

496–500

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

503

why do we need a special affordance for this

524

i'm not really seeing the point of this sugar

527

shouldn't we throw/fail validation?

611

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

637

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.

python_modules/libraries/dagster-pandas/dagster_pandas/constraints.py
146

good to note the default here

156

does this run the risk of collision

496–500

validators_for_columns?

578

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