Page MenuHomePhabricator

Use dagster type display name for dagster event message

Authored by rexledesma on Sep 22 2020, 8:33 PM.



Rather than using the name property of the dagster type, we use the display name, which is guaranteed to be non-null.

Test Plan

Existing tests

Diff Detail

R1 dagster
rl-fix-dagster-list-type-logging-display (branched from master)
Lint OK
No Unit Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 22 2020, 8:54 PM
Harbormaster failed remote builds in B18601: Diff 22581!

switch back List type to have name None


I don't think we should introduce this class just to set name = self.display_name (also don't think we should override display_name, type_check_method, inner_types, type_param_keys). I think your original suggestion of updating the error messages to use type_name=dagster_type.display_name instead of would be πŸ‘Œ


hmm, this is weird because ContainerDagsterType forces the type to have a name, so we should respect that here


This sort of solution is similar to how scalars subclass on BuiltinScalarDagsterType.

Updating the error messages to use type_name=dagster_type.display_name would only be a short term fix - then, in the future, developers must remember to use to .display_name in place of .name when printing dagster_type in string messages. I don't think that's sustainable.

Honestly, I think the best fix here would be to consolidate display_name and name together in one field somehow. DagsterType has this invariant where either key or name are required but then display_name must also be non-null, and that defaults to name so in the end, the actual invariant is both key and name must be provided, otherwise an error will show up.

Of course, the user can subclass DagsterType to override display_name to avoid this to pass in name as None but that doesn't seem like ergonomic behavior to me.


It looks like ContainerDagsterTypes (dict/set/list/optional) have no name so that this name uniqueness check is skipped for them, since they are usually constructed on the fly.

It should be fine to have names for these types as long as we remember to skip the check for them here.