Page MenuHomePhabricator

Delete all non *orError accessors
ClosedPublic

Authored by prha on Wed, May 13, 7:54 PM.

Details

Summary

We should have never had these in the first place

Test Plan

BK.

Click around dagit

  1. Playground
  2. Solid Viewer
  3. Pipeline Viewer

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.Wed, May 13, 7:54 PM
schrockn updated this revision to Diff 13913.Wed, May 13, 8:01 PM
schrockn edited the test plan for this revision. (Show Details)

up

Harbormaster returned this revision to the author for changes because remote builds failed.Wed, May 13, 8:14 PM
Harbormaster failed remote builds in B11321: Diff 13913!
schrockn requested review of this revision.Wed, May 13, 8:51 PM
schrockn added reviewers: max, yuhan, prha, bengotow.
schrockn edited the test plan for this revision. (Show Details)
schrockn updated this revision to Diff 13920.Wed, May 13, 8:59 PM
schrockn edited the test plan for this revision. (Show Details)

up

I *think* this behavior change is probably fine since the main content should be showing the guts of the error type - but would like @bengotow to vet this

maybe worth updating test plan to include forging an invalid url /pipeline/garbage_name or whatever to verify that the error state is sane in all parts of the ui

js_modules/dagit/src/SidebarSolidContainer.tsx
39โ€“70

hmm - is showing nothing on error here better than the toast popping up?

js_modules/dagit/src/execute/SolidSelector.tsx
40โ€“48

[1]

max added inline comments.Thu, May 14, 6:43 PM
js_modules/dagit/src/SidebarSolidContainer.tsx
39โ€“70

๐Ÿ‘

js_modules/dagit/src/execute/SolidSelector.tsx
108

should we take this opportunity to add (minimal) logging in all these cases where we previously were just doing data?.pipeline? - just a call to console.log would really help us debug remotely

schrockn planned changes to this revision.Sat, May 16, 9:55 PM

just rebased. will address feedback later

prha commandeered this revision.Tue, May 19, 11:12 PM
prha edited reviewers, added: schrockn; removed: prha.
prha updated this revision to Diff 14320.Wed, May 20, 9:09 PM

updated to add console.error calls, fetch error messages where useful.

Also, renamed ExecutionPlanResult to ExecutionPlanOrError for consistency.

Removed stray pickle test file

prha added a comment.Wed, May 20, 9:14 PM

In all of these dagit cases, we're fetching the pipeline in a sub-component where the container component has already fetched the pipeline.

We may hit an error case in which dagit gets into a bad state in between requests, or we hit a framework bug while resolving some downstream piece of data, but these should be extremely rare.

schrockn resigned from this revision.Wed, May 20, 10:16 PM

Looks good to me. Will let those more opinionated about the js weigh for final review.

max resigned from this revision.Wed, May 20, 10:35 PM

this lgtm, i will let @bengotow have the final word

alangenfeld accepted this revision.Wed, May 20, 10:41 PM

In all of these dagit cases, we're fetching the pipeline in a sub-component where the container component has already fetched the pipeline.
We may hit an error case in which dagit gets into a bad state in between requests, or we hit a framework bug while resolving some downstream piece of data, but these should be extremely rare.

in @prha we trust

thumbsup

This revision is now accepted and ready to land.Wed, May 20, 10:41 PM
prha updated this revision to Diff 14370.Wed, May 20, 11:13 PM

update tests, snapshots

This revision was automatically updated to reflect the committed changes.