Page MenuHomePhabricator

(new-config-parsing-6) More principled treatment of None in the config system no none
ClosedPublic

Authored by schrockn on Dec 15 2019, 8:20 PM.

Details

Summary

This has always been pretty fast and loose and I think actually
causes some ambiguities and confusing behavior. Now the system has a principled
view of things. If it's Nullable or Any, None is valid. Otherwise, it is not.

The corner case this exposes wis when we are dealing with selectors.
There are cases where the presence of a selector key just by itself is
enough information to know what configuration should be. In this case
we do accept None.

Depends on D1671

Test Plan

BK, dagit

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

schrockn created this revision.Dec 15 2019, 8:20 PM

This one is worth an in-person convo

nate added a comment.Dec 16 2019, 4:49 PM

per in-person discussion, will let Alex/Max chime in but LGTM!

hmm - is this problem really present outside Selector? Can we limit it to that somehow?

python_modules/dagster/dagster/utils/yaml_utils.py
14–36 ↗(On Diff #7771)

oofda this is a pretty generic call site for these shenanigans

schrockn added inline comments.Dec 16 2019, 5:30 PM
python_modules/dagster/dagster/utils/yaml_utils.py
36 ↗(On Diff #7771)

yup

schrockn updated this revision to Diff 7839.Dec 16 2019, 11:00 PM

another approach

schrockn updated this revision to Diff 7840.Dec 16 2019, 11:11 PM
schrockn retitled this revision from (new-config-parsing-6) More principled treatment of None in the config system no none to (new-config-parsing-6) More principled treatment of None in the config system no none.
schrockn edited the summary of this revision. (Show Details)

upmessage

This revision is now accepted and ready to land.Dec 16 2019, 11:29 PM