kotlin-inject icon indicating copy to clipboard operation
kotlin-inject copied to clipboard

Add a new annotation to generate Component accessors for KMP targets

Open eygraber opened this issue 1 year ago • 6 comments

Fixes #361 Fixes #371

eygraber avatar Mar 27 '24 16:03 eygraber

@evant naming is still not great, but I think it's kind of descriptive of what it is doing.

What do you think in terms of the module location, and moving the KSP test harness so that it could be used by both KSP modules?

eygraber avatar Mar 27 '24 16:03 eygraber

Oh one more thought, do we actually care about the companion here? Seems to me this is just a convenience in how the function is called, but

@TargetComponentAccessor
expect fun createMyCustomComponent(): MyCustomComponent

would work just as well. Id suggest blindly copying the function signature and not caring about if/what the receiver is.

evant avatar Mar 27 '24 20:03 evant

I was using the receiver to determine which create function to call, but I guess it makes more sense to use the return type. I also need to check if I should use the companion or KClass

eygraber avatar Mar 27 '24 21:03 eygraber

You can check me.tatarka.inject.generateCompanionExtensions to determine if you are generating kclass or companion calls

evant avatar Mar 27 '24 22:03 evant

Pushed all of those changes. I'll start writing tests tomorrow, but I did some refactoring to separate the Inject and TargetComponentAccessor processing, and wanted to get your thoughts on that.

eygraber avatar Mar 28 '24 03:03 eygraber

Added some documentation for KMP in general (mostly generated by ChatGPT, with some minor human intervention).

eygraber avatar Mar 29 '24 01:03 eygraber

@evant anything blocking this from getting merged?

eygraber avatar Apr 11 '24 01:04 eygraber

Added some documentation for KMP in general (mostly generated by ChatGPT, with some minor human intervention).

I took a quick look at it and it's confusing and unhelpful, I can't accept this in this state. I would go into details but I'm not spending more time on it than you took to generate it.

evant avatar Apr 17 '24 21:04 evant

Well the generation was pretty quick, but I did spend 30ish minutes tuning it, and trying to get the tone to match the other documentation. It's a difficult problem/solution to describe, and it's even more difficult to simplify.

Because I'm more familiar with the issue I have trouble seeing when it's confusing, so I was hoping to use this a springboard for feedback on what direction to take the docs, what to focus on, clarify, etc...

eygraber avatar Apr 17 '24 21:04 eygraber

@evant I updated the documentation to be fully human written :sweat_smile:

eygraber avatar Apr 19 '24 23:04 eygraber

Yeah much better lol. Actually there was a start of this already with https://github.com/evant/kotlin-inject/pull/339, I'm going to make some adjustments to it and merge it in first, then we can combine it with the new annotation instructions here. I'm also working on updating the greeter sample to better align, albeit with manual expect/actual for now.

evant avatar Apr 23 '24 17:04 evant

https://github.com/evant/kotlin-inject-samples/pull/19

evant avatar Apr 23 '24 17:04 evant

Ok merged, would suggest the last section should be replaced with annotation use, also feel free to pull anything else your wrote over if you feel it would be helpful. (I think the first section mentioning the tradoffs about generating for the common source set would be good too)

evant avatar Apr 23 '24 18:04 evant

I'll work on merging my docs in; hope to have it done some time next week.

I also thought of a much better annotation name than TargetComponentAccessor, but I forgot to write it down and I forgot it :see_no_evil:

While writing the docs it didn't feel like TargetComponentAccessor is really a great name. It might be technically correct, but I don't think it communicates its purpose to users. I'm going to spend some time trying to remember the better name that I though of.

Another thing I thought of was keeping it in a separate kmp runtime artifact, so that users not using kmp don't accidentally use it (it might also help with naming the annotation because it can be more on the nose about being for multiplatform). What do you think?

eygraber avatar Apr 26 '24 20:04 eygraber

While writing the docs it didn't feel like TargetComponentAccessor is really a great name

Yep, absolutely agree, I'll let you know if I think of anything too. After updating the sample project I do feel like I have a bit more handle on the problem than I did before.

Another thing I thought of was keeping it in a separate kmp runtime artifact

Hm, yeah it's a hard to know long-term which would be the right decision there, but that's something we can always change later if we need to so I say go for it.

evant avatar Apr 26 '24 21:04 evant

Pushed a bunch of updates. I made some changes to the existing docs. I'll annotate my reasoning there, but I'm fine to revert any of it if you feel that it shouldn't be changed.

eygraber avatar May 01 '24 22:05 eygraber

Addressed feedback, and waiting for decision on @KmpComponentCreator vs @CreateKmpComponent

eygraber avatar May 02 '24 04:05 eygraber

Let's go with CreateKmpComponent then

evant avatar May 02 '24 05:05 evant

Done!

eygraber avatar May 02 '24 06:05 eygraber

I thought about it a bit more and CreateKmpComponent is technically a misnomer because the Component isn't "KMP".

Maybe CreateComponentForKmp would be more accurate?

eygraber avatar May 02 '24 12:05 eygraber

Hm, yeah. I'm starting to second-guess my suggestion too, we are really focusing on the create here and not the Component. How about @KmpComponentCreate? I know it's basically the same as what you originally suggested heh.

evant avatar May 02 '24 18:05 evant

Haha that works for me. I'll push that in a couple of minutes.

eygraber avatar May 02 '24 18:05 eygraber