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

Scan more than one component at the time #182

Open
wants to merge 8 commits into
base: 2.0.0
Choose a base branch
from

Conversation

vivodikj
Copy link

@vivodikj vivodikj commented Oct 16, 2024

Summary

This PR create an option for add more packages in scanning component feature, inspired by this issue. With this change, developers can use new @ComponetsScan annotation with values parameter where they can add all needed packages. The original @ComponentScan annotations is 100% preserved and now can cooperate with new @ComponetsScan.

All tests still passes like a charm and new test passed without any problem.

Examples

Before

@Module(includes = [DataModule::class])
@ComponentScan("org.koin.sample.androidx.app")
class AppModule

@Module(includes = [CommonModule::class, RepositoryModule::class])
@ComponentScan("org.koin.sample.androidx.data")
internal class DataModule

After 1. variant

@Module(includes = [CommonModule::class, RepositoryModule::class])
@ComponentsScan(values = ["org.koin.sample.androidx.app", "org.koin.sample.androidx.data"])
class AppModule

After 2. variant

@Module
@ComponentsScan(values = [
    "org.koin.sample.androidx.app", "org.koin.sample.androidx.data",
    "org.koin.sample.androidx.repository", "org.koin.sample.android.library",
])
class AppModule

After 1. variant if we want scan actual package

@Module
@ComponentsScan(values = [
    "", "org.koin.sample.androidx.app", "org.koin.sample.androidx.data",
    "org.koin.sample.androidx.repository", "org.koin.sample.android.library",
])
class AppModule

After 2. variant if we want scan actual package

@Module
@ComponentsScan
@ComponentsScan(values = [
    "org.koin.sample.androidx.app", "org.koin.sample.androidx.data",
    "org.koin.sample.androidx.repository", "org.koin.sample.android.library",
])
class AppModule

NOTE:

If dev use @ComponentsScan or @ComponentsScan([]) or @ComponentsScan([""]) this will scan actual package like original @ComponentScan.

If dev add non existing or correct package name to @ComponentsScan values, the package will be ignored and nothing happens.

If dev add multiple package names to @ComponentsScan values, only one will be used and multiple package names error is not shown.

@arnaudgiuliani
Copy link
Member

let's go on branch v2 as it's a new feature / improvement proposal

@arnaudgiuliani arnaudgiuliani changed the base branch from 1.4.0 to 2.0.0 November 18, 2024 16:27
@arnaudgiuliani arnaudgiuliani added this to the 2.0 milestone Nov 18, 2024
@arnaudgiuliani
Copy link
Member

I propose to keep one dedicated annotation, with @ComponentScan. Let's use the values atrtibutes to gather array of package.

No doubling of @ComponentScan needed. If people wants to go with current package, let's keep it "".

Can you update your code? 🙏

@vivodikj
Copy link
Author

vivodikj commented Dec 26, 2024

@arnaudgiuliani
I have modified the code, including the examples. The user can use @ComponentScan without additional parameters or "" to scan the current package. I also added a trim to each scanned package, so there shouldn't be a problem with, for example " ", ".... ", " ......", " ..... ".

*/
@Target(AnnotationTarget.CLASS, AnnotationTarget.FIELD)
annotation class ComponentScan(val value: String = "")
annotation class ComponentScan(val values: Array<String> = [""])
Copy link
Member

Choose a reason for hiding this comment

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

could be empty array here?

Copy link
Member

Choose a reason for hiding this comment

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

or perhaps useless 🤔

Copy link
Author

Choose a reason for hiding this comment

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

oki, I'll remove default empty string value.

@arnaudgiuliani
Copy link
Member

CI failed. Can you look at this? @vivodikj :

e: file:///Users/runner/work/koin-annotations/koin-annotations/examples/android-coffee-maker/build/generated/ksp/debug/kotlin/org/koin/ksp/generated/KoinDefault-563727094.kt:32:2 Unresolved reference 'defineScopedStuff'.
> Task :android-coffee-maker:compileDebugKotlin FAILED
e: file:///Users/runner/work/koin-annotations/koin-annotations/examples/android-coffee-maker/src/main/java/org/koin/sample/androidx/di/AppModules.kt:18:16 No parameter with name 'values' found.

@vivodikj
Copy link
Author

vivodikj commented Jan 21, 2025

@arnaudgiuliani about the CI failed. It's look like CI don't use new "updated" version of coin annotations. Maybe they don't use created mavenLocal repository but mavenCentral repository. Where is not new componentScan annotation with values array.

We can edit build.gradle.kts (examples) to first check for dependencies in mavenLocal and then in mavenCentral.
Edit from:

allprojects {
    repositories {
        google()
        mavenCentral()
        mavenLocal()
    }
}

To:

allprojects {
    repositories {
        mavenLocal()
        google()
        mavenCentral()
    }
}

@arnaudgiuliani
Copy link
Member

@vivodikj I'm about to merge your PR. We have a conflict in the sample project

@vivodikj
Copy link
Author

@arnaudgiuliani No more conflict :)

@arnaudgiuliani
Copy link
Member

Great job 👍

@arnaudgiuliani
Copy link
Member

arnaudgiuliani commented Jan 27, 2025

I'm sorry. I'm gathering all the release stuff. The problem is that we are breaking compatibility on @componentscan. This was also your proposal to add another annotation. :/ 🙏

Let's find how too mitigate that. We could use an extra Annotation property? 🤔
This PR is also bringing capacity on Scanning: #131

@vivodikj
Copy link
Author

Yes, that's why I created another annotation in the first place.

We could add extra new property to existing @componentScan annotation, something like nullable string array values? Or we can create new annotation as before.

Ya, I think this PR with #131 will be very good combination.

@vivodikj
Copy link
Author

@arnaudgiuliani Maybe I found a pretty good solution. We can make value in @ComponentScan vararg. Which will look like annotation class ComponentScan(vararg val value: String = []) and usage can be for for example @ComponentScan("....", ".....", "....."), @ComponentScan("...."), @ComponentScan also @ComponentScan(value = ["...", "..."]) or @ComponentScan(value = ["..."]). But there don't work named parameter this way @ComponentScan(value = "...") only this @ComponentScan(value = ["..."]) or @ComponentScan(".....").

@arnaudgiuliani
Copy link
Member

Yeah, clearly. Let's add the paths attribute like this:

/**
 * Gather definitions declared with Koin definition annotation
 * Will scan in the current package or an explicit package path name
 * Also can scan across multiple package paths. 
 *
 * @param value: path to scan
 * @param paths : list of path to scan
 */
@Target(AnnotationTarget.CLASS, AnnotationTarget.FIELD)
annotation class ComponentScan(val value: String = "", val paths: Array<String> = [])

What do you think?

@arnaudgiuliani
Copy link
Member

@arnaudgiuliani Maybe I found a pretty good solution. We can make value in @ComponentScan vararg. Which will look like annotation class ComponentScan(vararg val value: String = []) and usage can be for for example @ComponentScan("....", ".....", "....."), @ComponentScan("...."), @ComponentScan also @ComponentScan(value = ["...", "..."]) or @ComponentScan(value = ["..."]). But there don't work named parameter this way @ComponentScan(value = "...") only this @ComponentScan(value = ["..."]) or @ComponentScan(".....").

our messages get crossed. Yeah interesting proposal. Not sure people used @componentscan with value property 🤔

@vivodikj
Copy link
Author

Me and my team don't use @ComponentScan with named value property but only @ComponentScan("..."). But if someone use named value property, they can delete it and everything will works. I think this is little bit cleaner to use, but if you like your proposal more I will make it. Its up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants