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.

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

[graphql] refactor node def, config, and type resolution from assets #25552

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alangenfeld
Copy link
Member

go through the context for resolving pieces of information to facilitate different backing implementations in other workspace classes.

How I Tested These Changes

existing tests

Copy link
Member Author

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

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

@alangenfeld alangenfeld force-pushed the al/cached_nodes branch 2 times, most recently from 4364545 to 592d0df Compare October 25, 2024 20:03
Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

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

Gnarly but impressive. I think we may still also want to defer the definitions fields that are using these types? It seems easy to accidentally slip in a new callsite to the node

@@ -391,36 +391,42 @@ def stale_status_loader(self) -> StaleStatusLoader:
def asset_graph_differ(self) -> Optional[AssetGraphDiffer]:
return self._asset_graph_differ

def _job_selector(self) -> Optional[JobSelector]:
if len(self._asset_node_snap.job_names) < 1:
Copy link
Member

Choose a reason for hiding this comment

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

== 0?

@@ -19,28 +19,26 @@


def to_config_type(
config_schema_snapshot: ConfigSchemaSnapshot, config_type_key: str
get_config_type: Callable[[str], ConfigTypeSnap],
Copy link
Member

Choose a reason for hiding this comment

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

maybe worth a comment about why this is deferred?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants