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

[components] Put resolvers back on ResolvableSchema #27734

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

Conversation

OwenKephart
Copy link
Contributor

Summary & Motivation

Reviving some ideas that we strayed away from, remixed slightly.

Some nice things about this iteration:

  1. No magic indirection. There are no special method names that automatically get resolved -- you just call resolve_fields() to get a dictionary of all the resolved fields, and you can optionally exclude some or add some more in as desired (see: AssetSpec). This means that stack traces will be pretty
  2. Good type system support. Because the schema objects know what type they're supposed to turn into, we know what kind of thing we'll get when we call context.resolve(self.assets). This means that if we plug this value into an incompatible spot, the type system can warn us ahead of time. I've also set the overloads up such that if the type system can't infer what type something will turn into (i.e. you're just resolving an arbitrary string), you're forced to put the as_type parameter in to tell us what you think it will be. I didn't actually wire this up to any checking code yet, but that's a natural extension.
  3. Pretty terse. If you make good use out of get_resolved_fields(), there's really not a ton of boilerplate.

Some bad things about this iteration:

  1. Why do we override resolve on some schemas and resolve_as on others? This is because we need to support the case where we create a subclass of a Component. If we just made the schema directly resolve into (e.g.) DbtProjectComponent, then load() would always return DbtProjectComponent instead of the subclass that you actually want.
  2. This actually messes with the type system (for some reason), which can't recognize that cls(not_a_field=1) is invalid when done inside of the resolve_as() method (try as I might to type the input parameter). In contrast, if you do the resolution inside of the load() method, things work out just fine.

One solution here might just be convention. We could create a ResolvedDbtProjectSchema type, and have DbtProjectSchema resolve into that, then define an explicit load() method on DbtProjectComponent that looks like:

@classmethod
def load(cls, schema: DbtProjectSchema, context):
    resolved_schema = schema.resolve(context)
    return DbtProjectComponent(
        resource=resolved_schema.resource,
        translator=resolved_schema.translator,
        ...
    )

That way, we'd be able to get rid of the resolve_as method entirely.

How I Tested These Changes

Changelog

NOCHANGELOG

Comment on lines +89 to +91
def load(
cls, context: ResolutionContext, schema: SlingReplicationCollectionParams
) -> "SlingReplicationCollection":
Copy link

Choose a reason for hiding this comment

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

The method signature needs to be updated to match the base class signature:

def load(cls, schema: SlingReplicationCollectionParams, context: ResolutionContext) -> "SlingReplicationCollection":

The parameter order is currently reversed.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Contributor Author

OwenKephart commented Feb 11, 2025

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.

1 participant