Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Factor out strange resource-related logic into helper class RelatedResourceState #22243

Merged
merged 1 commit into from
Jun 7, 2024
Merged

Factor out strange resource-related logic into helper class RelatedResourceState #22243

merged 1 commit into from
Jun 7, 2024

Conversation

schrockn
Copy link
Member

@schrockn schrockn commented Jun 3, 2024 •
edited
Loading

Summary & Motivation

There is super convoluted logic when we pass in an io_manager_def directly. This captures that in a class and explicitly documents it.

How I Tested These Changes

BK

This was referenced Jun 3, 2024
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@schrockn Graphite App
Copy link
Member Author

schrockn commented Jun 3, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @schrockn and the rest of your teammates on Graphite Graphite

This was referenced Jun 3, 2024
Merged
Closed
@schrockn schrockn changed the title Factor out RelatedResourceState Factor out strange resource-related logic into helper class RelatedResourceState Jun 3, 2024
@schrockn schrockn requested review from benpankow, smackesey and sryza June 3, 2024 15:21
@schrockn schrockn marked this pull request as ready for review June 3, 2024 15:21
@graphite-app graphite-app bot added the area: docs Related to documentation in general label Jun 3, 2024
@graphite-app graphite-app bot requested a review from erinkcochran87 June 3, 2024 15:41
@schrockn schrockn mentioned this pull request Jun 3, 2024
Merged
@schrockn schrockn force-pushed the aget-multi-asset-refactor-13 branch from 687ebc0 to 2316846 Compare June 3, 2024 15:46
@schrockn schrockn force-pushed the aget-multi-asset-refactor-true-14 branch from edc6c22 to c9624a2 Compare June 3, 2024 15:46
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 3, 2024

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-hqvnwlckh-elementl.vercel.app
https://aget-multi-asset-refactor-true-14.dagster.dagster-docs.io

Direct link to changed pages:

@schrockn schrockn force-pushed the aget-multi-asset-refactor-13 branch from 2316846 to 4964912 Compare June 3, 2024 18:37
@schrockn schrockn force-pushed the aget-multi-asset-refactor-true-14 branch from c9624a2 to f49250b Compare June 3, 2024 18:37
@schrockn schrockn mentioned this pull request Jun 3, 2024
Merged
@schrockn schrockn force-pushed the aget-multi-asset-refactor-13 branch from 4964912 to eee94c4 Compare June 4, 2024 00:56
@schrockn schrockn force-pushed the aget-multi-asset-refactor-true-14 branch from f49250b to 45e0e28 Compare June 4, 2024 00:56
@schrockn schrockn force-pushed the aget-multi-asset-refactor-13 branch from eee94c4 to 31fb5ac Compare June 4, 2024 01:02
@schrockn schrockn force-pushed the aget-multi-asset-refactor-true-14 branch from 45e0e28 to 3d349fe Compare June 4, 2024 01:02
benpankow
benpankow approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@benpankow benpankow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, much nicer

@schrockn schrockn mentioned this pull request Jun 4, 2024
Merged
This was referenced Jun 8, 2024
Merged
Closed
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Merged
Merged
Closed
Merged
Merged
Closed
Merged
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
danielgafni pushed a commit to danielgafni/dagster that referenced this pull request Jun 18, 2024
@schrockn @danielgafni
Factor out strange resource-related logic into helper class RelatedRe…
75cd213 
…sourceState (dagster-io#22243)

## Summary & Motivation

There is super convoluted logic when we pass in an `io_manager_def` directly. This captures that in a class and explicitly documents it.

## How I Tested These Changes

BK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpankow benpankow benpankow approved these changes

@sryza sryza sryza approved these changes

@smackesey smackesey Awaiting requested review from smackesey

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@schrockn @sryza @benpankow @erinkcochran87