Page MenuHomePhabricator

Kill dagster.Path
ClosedPublic

Authored by schrockn on Sun, May 10, 4:28 PM.

Details

Summary

We killed Path in the config system but left it in the dagster
type system, which was a mistake. This just moves forward with totally
eliminating Path.

Alternatives would be including a tombstone object and then erroring on
it with a specific error message, because right now the error message
can be a bit rough: https://dagster.slack.com/archives/CCCR6P2UR/p1589121638025100

Test Plan

BK

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.Sun, May 10, 4:28 PM
schrockn requested review of this revision.Sun, May 10, 5:00 PM
schrockn updated this revision to Diff 13589.Sun, May 10, 5:35 PM
schrockn retitled this revision from kill dagster.Path to Kill dagster.Path.
schrockn edited the summary of this revision. (Show Details)
schrockn added reviewers: max, alangenfeld, prha.

up

we should probably have some way to express something akin to Path eventually, ideally at a specificity that we can actually do meaningful validation - existence checks, access checks, or even just format checks.

python_modules/dagster/dagster/core/types/dagster_type.py
309–319

huh we weren't doing any extra validation? I guess the fact that this was misleading warrants removal.

schrockn added inline comments.Mon, May 11, 11:54 PM
python_modules/dagster/dagster/core/types/dagster_type.py
309–319

yeah that's why i think this should go away

max accepted this revision.Tue, May 12, 1:24 AM

Let's get rid of it

This revision is now accepted and ready to land.Tue, May 12, 1:24 AM

add a pending changed.md entry so we dont forget

This revision was automatically updated to reflect the committed changes.