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

Parse the TargetResolver from assemblies #94

Merged
merged 4 commits into from
Nov 15, 2023
Merged

Conversation

skorulis-ap
Copy link
Collaborator

@skorulis-ap skorulis-ap commented Nov 9, 2023

Looks for instances of typealias TargetResolver = Foo and generates type safe extensions underneath the targeted resolver.

This allows correct scoping of generated type safety functions.

I added useTargetResolver as a way to disable this functionality if we upgrade knit versions but aren't ready to make the switch.

defaultExtensionTarget was also added to allow correctly setting the resolver for assemblies which aren't explicitly defining a typealias.

@skorulis-ap skorulis-ap marked this pull request as ready for review November 9, 2023 18:26
@skorulis-ap skorulis-ap force-pushed the skorulis/target-resolver branch 2 times, most recently from 57250be to 4b84bf2 Compare November 9, 2023 18:29
Copy link
Collaborator

@bradfol bradfol left a comment

Choose a reason for hiding this comment

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

A couple small suggestions. LGTM!


public init(
name: String,
registrations: [Registration],
registrationsIntoCollections: [RegistrationIntoCollection],
imports: [ImportDeclSyntax] = []
imports: [ImportDeclSyntax] = [],
targetResolver: String = "Resolver"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the default value here? (so it is not specified in multiple places)

var useTargetResolver: Bool = false

@Option(help: "Default type to extend when generating Resolver type safety methods")
var defaultExtensionTarget = "Resolver"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe defaultExtensionTargetResolver to keep naming consistent

self.targetResolver = identifier.name.text
}

return .skipChildren
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just as a curiosity I wonder if it is possible for TypeAliasDecl nodes to have child nodes 🤔

@skorulis-ap skorulis-ap force-pushed the skorulis/target-resolver branch from 82ba97b to b201b3d Compare November 15, 2023 00:51
self.targetResolver = identifier.name.text
}

return .visitChildren
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh sorry if my other comment was confusing, I think skip children is what we want here

@skorulis-ap skorulis-ap force-pushed the skorulis/target-resolver branch from b201b3d to 52a80a4 Compare November 15, 2023 01:49
@skorulis-ap skorulis-ap merged commit b95eea1 into main Nov 15, 2023
1 check passed
@skorulis-ap skorulis-ap deleted the skorulis/target-resolver branch November 15, 2023 02:18
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.

3 participants