Page MenuHomePhabricator

Make anonymous types in config type system have stable keys

Authored by schrockn on Nov 20 2019, 3:32 PM.



This has been a long-standing technical liability that IMO
become intolerable with the upcoming work to persist the type system
metadata in a database. Instead of just assigning an incremental
counter to Dicts, Selectors, etc to assign key names (so that we
can do things like have the config editor work) instead this
assigns the key based on the fields passed into types.

Critically this makes it so that import order will no longer alter
the keys.

Test Plan

BK and run dagit.

Diff Detail

R1 dagster
Lint OK
No Unit Test Coverage

Event Timeline

schrockn created this revision.Nov 20 2019, 3:32 PM
schrockn updated this revision to Diff 6740.Nov 20 2019, 3:56 PM
schrockn retitled this revision from make dictionary keys stable to Make anonymous types in config type system have stable keys.
schrockn edited the summary of this revision. (Show Details)
schrockn edited the test plan for this revision. (Show Details)


Probably a good idea to do this after:

schrockn updated this revision to Diff 6742.Nov 20 2019, 4:46 PM

test field order does not matter

alangenfeld requested changes to this revision.Nov 20 2019, 5:37 PM

test for recursive /deeply nested dict structure

This revision now requires changes to proceed.Nov 20 2019, 5:37 PM

otherwise looks solid

max resigned from this revision.Nov 20 2019, 7:19 PM

agree with alex we should be rigorous about test coverage but this is such a good thing to finally put to bed. resigining in favor of @alangenfeld πŸ‘


specifically, i'd like to make sure *this* is still true for deeply nested structures

schrockn updated this revision to Diff 6753.Nov 20 2019, 7:41 PM

nested mcnestyface

This revision is now accepted and ready to land.Nov 20 2019, 11:13 PM