mcarleio / konvert

This kotlin compiler plugin is using KSP API and generates kotlin code to map one class to another
https://mcarleio.github.io/konvert/
Apache License 2.0
86 stars 8 forks source link

Add Koin injector #4

Closed jakoss closed 1 year ago

jakoss commented 1 year ago

Implements adding Koin annotations defined in here: https://insert-koin.io/docs/reference/koin-annotations/definitions

There is one issue @mcarleio - i believe it's something with the proxy. When i'm trying to pass TestScope::class as annotation parameter the ksp fails (cannot resolve that class name). There is a failing test with repro named factoryScopedWithClass, please take a look at a spare time.

As to injectors themselves - super solution, adding new one was very easy. There are two features i'd love to have:

Also it would be beneficial to run build action in pull requests too :)

mcarleio commented 1 year ago

Thank you for the PR!

There is one issue @mcarleio - i believe it's something with the proxy. When i'm trying to pass TestScope::class as annotation parameter the ksp fails (cannot resolve that class name). There is a failing test with repro named factoryScopedWithClass, please take a look at a spare time.

Yeah, I had the same problem with e.g. @KonvertTo and could only solve it, by not using getAnnotationsByType. That is the main reason for the AnnotationData. The problem is, that you can reference a class, which is being compiled at the same time and then the ksp method is sadly not working as expected.

  • Pass ksp options to injector, so we can use some options directly in injectors themselves. This is related to the next feature
  • Add option to specify default injecting annotation for all mappers. For example - i know the 99% of my mappers will be simply annotated with a @Factory without any additions. So instead of adding @KFactory to every single interface i create maybe it would be nice to add some ksp option, like konvert.koin-injector.default-annotation: @KFactory

This should already be possible, but is not documented well. You can simply access the configuration via

import io.mcarle.konvert.converter.api.config.Configuration

Configuration.CURRENT.get("konvert.koin-injector.default-annotation")

I think it would be good to have all possible configuration keys in KonvertOptions. To simplify usage, I would recommend to implement something like this and use it in your injector.

Also it would be beneficial to run build action in pull requests too :)

I would have thought that GitHub will show a "Approve and run" button in this PR, because you are a First-time contributor here. But there is no button and I am unsure why. I suspect the

permissions:
      packages: write

part, so maybe I have to extract the release part from the build.yml.

jakoss commented 1 year ago

There is one issue @mcarleio - i believe it's something with the proxy. When i'm trying to pass TestScope::class as annotation parameter the ksp fails (cannot resolve that class name). There is a failing test with repro named factoryScopedWithClass, please take a look at a spare time.

Yeah, I had the same problem with e.g. @KonvertTo and could only solve it, by not using getAnnotationsByType. That is the main reason for the AnnotationData. The problem is, that you can reference a class, which is being compiled at the same time and then the ksp method is sadly not working as expected.

I'll look at that tomorrow.

I would have thought that GitHub will show a "Approve and run" button in this PR, because you are a First-time contributor here. But there is no button and I am unsure why. I suspect the

permissions:
      packages: write

part, so maybe I have to extract the release part from the build.yml.

Don't you need to add

on:
    pull_request:
        types: [ opened, synchronize, reopened ]

to your workflow?

mcarleio commented 1 year ago

Don't you need to add

on:
    pull_request:
        types: [ opened, synchronize, reopened ]

to your workflow?

Oh, you are right, I have no pull_request defined... I just added it (and split the build and release workflows) - hope it works now :+1:

jakoss commented 1 year ago

Yeah, I had the same problem with e.g. @KonvertTo and could only solve it, by not using getAnnotationsByType. That is the main reason for the AnnotationData. The problem is, that you can reference a class, which is being compiled at the same time and then the ksp method is sadly not working as expected.

I think i figured it out, but please take a look if you can spot something fishy.

I can add the default factory injectors in another PR

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 88.46% and project coverage change: +0.08 :tada:

Comparison is base (76e51e6) 82.13% compared to head (c851cb1) 82.22%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #4 +/- ## ========================================== + Coverage 82.13% 82.22% +0.08% ========================================== Files 56 57 +1 Lines 1819 1845 +26 Branches 260 265 +5 ========================================== + Hits 1494 1517 +23 Misses 169 169 - Partials 156 159 +3 ``` | [Impacted Files](https://codecov.io/gh/mcarleio/konvert/pull/4?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Marcel) | Coverage Δ | | |---|---|---| | [...in/io/mcarle/konvert/injector/koin/KoinInjector.kt](https://codecov.io/gh/mcarleio/konvert/pull/4?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Marcel#diff-aW5qZWN0b3JzL2tvaW4taW5qZWN0b3Ivc3JjL21haW4va290bGluL2lvL21jYXJsZS9rb252ZXJ0L2luamVjdG9yL2tvaW4vS29pbkluamVjdG9yLmt0) | `88.46% <88.46%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

mcarleio commented 1 year ago

I think i figured it out, but please take a look if you can spot something fishy.

Feels a bit strange to have to handle the annotations like that, but I also have no other good idea 🤔

Could you please amend the KoinInjector to do the same logic for these examples:


To have to test coverage collected for the new modules, please amend the root build.gradle.kts and add

kover(project(":injectors:koin-annotations"))
kover(project(":injectors:koin-injector"))
jakoss commented 1 year ago

Sure, will do

jakoss commented 1 year ago

@mcarleio I'm just running both methods on all annotations now, i think that covers all of possible use cases