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

Refactor step3codegen package (mainly TypeGenerator) #72

Merged
merged 3 commits into from
Oct 24, 2022

Conversation

myhau
Copy link
Member

@myhau myhau commented Oct 17, 2022

Task

Related to #24.

Description

Note

I could do much more here, but I've already proved to myself that these big-bang refactorings turn into never-ending rabbit holes (with lots of merge conflicts). We need to keep these PRs small and focused.

Main changes

1.

Refactor TypeGenerator:

2.

Add comments to some methods that return TypeName or FunSpec. Example:

/**
* Example output:
*
* ```
* List<suspend InnerType.() -> Unit>
* ```
*/
fun listOfLambdas(innerType: TypeName): TypeName =
LIST.parameterizedBy(builderLambda(innerType))

3.

Create KotlinPoetExtensions.kt files with extension methods that could increase readability (e.g. addImports) and reduce noise in generators' code. Example:

object KotlinPoetExtensions {
fun FileSpec.Builder.addTypes(vararg typeSpecs: TypeSpec) =
addTypes(typeSpecs.toList())
fun TypeSpec.Builder.addFunctions(vararg funSpecs: FunSpec) =
addFunctions(funSpecs.toList())

4.

Add new methods and rename existing methods in MoreTypes. For example:

object MoreTypes {
object Kotlin {
object Pulumi {
fun toJavaMethod(): MemberName {
return MemberName("com.pulumi.kotlin", "toJava")
}
fun pulumiDslMarkerAnnotation(): ClassName {
return ClassName("com.pulumi.kotlin", "PulumiTagMarker")
}

We can use something::class.asTypeName(), but I'm not sure this will be possible in the future (#41), so I'd prefer having all these "hardcoded" cases in MoreTypes for now.

(Edit) Clarification from: #72 (comment)

I turned all ClassName(package, name) into Type::class.asClassName(), but I'd still prefer to keep them all (MemberName and ClassName factory methods, like applySuspendExtensionMethod) in this class (MoreTypes) to ease potential refactorings and have opportunity to give them good names.

(MoreTypes should probably be named differently and placed in step3codegen package.)

Merging this PR

I created main-2022-10-16-all-pr-merge branch and cherry-picked existing PRs there to minimize the risk of conflicts, so this can be merged only after these are merged:

@myhau myhau force-pushed the 24-refactor-step3codegen-2022-10-16 branch from f936b32 to 8616eb8 Compare October 17, 2022 11:47
@myhau myhau self-assigned this Oct 17, 2022
@myhau myhau marked this pull request as ready for review October 17, 2022 11:51
@myhau myhau requested a review from a team as a code owner October 17, 2022 11:51
@myhau myhau requested review from jplewa and ddzikon and removed request for a team October 17, 2022 11:51
@myhau myhau changed the base branch from main-2022-10-16-all-pr-merge to main October 18, 2022 11:36
@myhau myhau force-pushed the 24-refactor-step3codegen-2022-10-16 branch from 8616eb8 to e42b191 Compare October 18, 2022 11:48
Copy link
Member

@jplewa jplewa left a comment

Choose a reason for hiding this comment

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

Just some minor stuff. Also, you need to get rebased.

@jplewa
Copy link
Member

jplewa commented Oct 18, 2022

@myhau

We can use something::class.asTypeName(), but I'm not sure this will be possible in the future (#41), so I'd prefer having all these "hardcoded" cases in MoreTypes for now.

I think we could still attach that library as a dependency here, no?

@myhau myhau force-pushed the 24-refactor-step3codegen-2022-10-16 branch from e42b191 to 598affc Compare October 19, 2022 08:16
@myhau myhau force-pushed the 24-refactor-step3codegen-2022-10-16 branch from 598affc to 7435731 Compare October 19, 2022 10:33
@myhau
Copy link
Member Author

myhau commented Oct 20, 2022

@myhau

We can use something::class.asTypeName(), but I'm not sure this will be possible in the future (#41), so I'd prefer having all these "hardcoded" cases in MoreTypes for now.

I think we could still attach that library as a dependency here, no?

Yes, I just turned all ClassName(package, name) into Type::class.asClassName(). I'd still prefer to keep them all (MemberName and ClassName factory methods, like applySuspendExtensionMethod) in this class to ease potential refactorings and have opportunity to give them good names.

@myhau myhau force-pushed the 24-refactor-step3codegen-2022-10-16 branch from 0aae79d to e8c2697 Compare October 20, 2022 14:23
@myhau myhau force-pushed the 24-refactor-step3codegen-2022-10-16 branch from e8c2697 to 896cde2 Compare October 20, 2022 14:28
@myhau myhau requested a review from jplewa October 20, 2022 14:29
# Conflicts:
#	src/main/kotlin/com/virtuslab/pulumikotlin/codegen/step3codegen/resources/ResourceGenerator.kt
@myhau myhau merged commit 0e34611 into main Oct 24, 2022
@myhau myhau deleted the 24-refactor-step3codegen-2022-10-16 branch October 24, 2022 11:40
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