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

🏗️ Add KSP artifact #61

Merged
merged 4 commits into from
Nov 1, 2023
Merged

🏗️ Add KSP artifact #61

merged 4 commits into from
Nov 1, 2023

Conversation

heytherewill
Copy link
Contributor

@heytherewill heytherewill commented Oct 30, 2023

Summary

This adds the base artifact and the first Symbol Processor for this project

Relationships

Groundwork for #60

Technical considerations

This PR is just the baseline and the code generation for the actions. I'll create subsequent issues for the State and Pullback generation

Tests

I'm testing both generation and the compiler error.

Review pointers

Please review not only the generated code's behavior, but also the overall architecture and ergonomics of the library, specially when applied to actual code.

Merge permissions

Go for it!

@semanticer
Copy link
Collaborator

semanticer commented Oct 31, 2023

@heytherewill Nice! KSP and Poet look great together. I didn't expect to understand the code soo easily 😅

There seems to be a bug tho. When I copy SettingsAction and AppAction to Todos sample I get this code generated:

public fun mapAppActionToSettingsAction(appAction: AppAction): SettingsAction? = if(appAction is
    .AppAction.Settings) appAction.settingsAction else null

public fun mapSettingsActionToAppAction(settingsAction: SettingsAction): AppAction.Settings =
    .AppAction.Settings(settingsAction)

There is that extra dot there in front of .AppAction.Settings

It looks like adding a +1 here:

val parentActionTypeName = parentActionClassName.canonicalName.substring(parentActionClassName.packageName.length + 1, parentActionClassName.canonicalName.length)

Solves the issue but idk if it's the most robust solution to the problem.

@heytherewill
Copy link
Contributor Author

Nice find. My original problem was that just getting the class name would not resolve nested classes properly. Adding the +1 like you suggested would break things for classes without a package. .trim('.') (plus adding a test) was the way to go

Copy link
Collaborator

@semanticer semanticer left a comment

Choose a reason for hiding this comment

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

Works great, it handled everything I threw at it. But it's not closing #60 perhaps we should turn #60 into checklist with a bunch of sub issues.

@heytherewill
Copy link
Contributor Author

Good point! Updated the comment, merge away!

@semanticer semanticer merged commit ee22dac into main Nov 1, 2023
12 checks passed
@semanticer semanticer deleted the will/ksp branch November 1, 2023 13:50
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