Added the ability to use eternal plugins with forma #75

ikarenkov · 2021-01-14T17:04:43Z

Added the ability to use eternal plugins with forma.
Also added platform dependency type.

…m dependency type

ikarenkov · 2021-01-14T17:06:02Z

ikarenkov · 2021-01-14T17:09:38Z

plugins/android/src/main/java/tools/forma/android/plugin/PluginWrapper.kt

+package tools.forma.android.plugin
+
+import tools.forma.deps.FormaDependency
+import emptyDependency


unused import

ikarenkov · 2021-01-14T17:14:19Z

plugins/android/src/main/java/tools/forma/android/plugin/PluginWrapper.kt

+import org.gradle.kotlin.dsl.the
+import tools.forma.deps.applyDependencies
+import tools.forma.validation.EmptyValidator
+import Forma


unused import

stepango · 2021-01-14T20:21:01Z

application/buildSrc/src/main/java/Plugins.kt

+import com.google.firebase.crashlytics.buildtools.gradle.CrashlyticsExtension
+import tools.forma.android.plugin.PluginWrapper
+
+object pluginWrappers {


How about Plugins instead of pluginWrappers?

Gradle already has plugins and will get naming collision

You can name just object Plugins, cause suffix Wrappers doesn't describe behavior of that class.
Also, I has checked that, we don't get collision with Upper case

we used lower case before, is it good idea to allow both naming styles? Maybe for Plugins it applicable but we need think about code style

stepango · 2021-01-14T20:22:27Z

plugins/android/src/main/java/FormaBuilder.kt

+            PluginWrapper(pluginId, pluginExtension, dependencies, pluginConfiguration)
+    )
+
+    // TODO: maybe should crate withPlugins(vararg pluginWrapper: PluginWrapper<*>)


stepango · 2021-01-14T20:23:53Z

plugins/android/src/main/java/FormaBuilder.kt

+import tools.forma.android.plugin.PluginWrapper
+import kotlin.reflect.KClass
+
+class FormaBuilder(


How about TargetBuilder here?

stepango · 2021-01-14T20:24:36Z

plugins/android/src/main/java/androidLibrary.kt

-import tools.forma.android.feature.kotlinAndroidFeatureDefinition
-import tools.forma.android.target.LibraryTargetTemplate
+import org.gradle.api.Project
+import tools.forma.android.feature.*


please avoid wildcard imports

It is just default code style settings from forma project, it brakes sometimes even if I set my own setting before. We should share code style for project using .idea

Of course I'll fix it)

You can commit project settings in the repo as well

stepango · 2021-01-14T20:27:24Z

plugins/android/src/main/java/FormaBuilder.kt

+        private val project: Project
+) {
+
+    fun <TPluginExtension : Any> withPlugin(


what is the use-case for this function?
IMHO having full plugin configuration in target config is not desirable.

I Agree, I'll remove it

stepango · 2021-01-14T20:31:11Z

application/core/navigation/library/build.gradle.kts

Should we establish a good practice by creating

androidLibraryWithNavigation(...) = androidLibrary(...).withPlugin(...)

wrapper function, so we'll make targets description in the same style?

IMHO, creating new method for each combination of plugins is useless, it doesn't sole our tasks. We can use plugins for several targets? f.e. Kotlin.serialization can be used in library, utils, iml, androidLibrary, androidUtils and in this case we must create 5 copipaste of methods, if I understand you correctly. Also we can have combinations of plugins. In my opinion patterns like builder or decorator help us to combine several behaviors and keep it scalable without necessity to duplicate code

Yea, in modern projects, we'll get different combinations of custom plugins. In that case, many methods with name templates as `[target name]With[combination plugins] will be overhead for project self.

stepango · 2021-01-14T20:35:39Z

plugins/android/src/main/java/tools/forma/android/plugin/PluginWrapper.kt

+) {
+
+    init {
+        if (pluginConfiguration != null) {


instead of doing this check, we can create a data class that will contain plugin extension and plugin configuration so we can reuse it later if, for example, different targets would need a slightly different configuration. This way we make sure Plugin Extension and Plugin configuration will always be specified together + compile-time check instead of runtime.

yep, nice idea

stepango · 2021-01-14T20:36:31Z

plugins/deps-core/src/main/java/tools/forma/deps/ConfigurationType.kt

@@ -31,7 +31,8 @@ class FileSpec(
 class NameSpec(
    val name: String,
    config: ConfigurationType,
-    val transitive: Boolean = false
+    val transitive: Boolean = false,
+    val platform: Boolean = false


Should we do a PlatformSpec instead?

I'm not sure, because it still nameSpec with specific behavior of resolving dependencies, platform is similar with transitive from this point of view. But it was just simple way to add platform, maybe we can add one more Spec and use it

Single type for it will be more clearly, than added overriding versions of transitive methods for passing one changing value of argument.

michaem · 2021-01-15T09:42:24Z

application/build.gradle.kts

@@ -5,6 +5,7 @@ buildscript {
    }
    dependencies {
        classpath("androidx.navigation:navigation-safe-args-gradle-plugin:2.3.2")
+        classpath("com.google.firebase:firebase-crashlytics-gradle:2.4.1")


We still have native gradle script part here. I think need to another issue to research, how could migrate on Forma API only

michaem · 2021-01-15T10:17:37Z

plugins/deps-core/src/main/java/tools/forma/deps/DependencyHandler.kt

@@ -36,6 +36,18 @@ fun DependencyHandler.addDependencyTo(
    configuration
 )

+// TODO: only using for platform, maybe make addPlatformDependencyTo?
+fun DependencyHandler.addDependencyTo(


Whats real different of this method from same signature upper? Is't only in one parameter Dependency instead on String?

michaem · 2021-01-15T10:32:28Z

plugins/deps-core/src/main/java/dependencies.kt

+
+fun transitivePlatform(vararg names: String): NamedDependency = transitiveDeps(names = *names, transitive = true, platform = true)
+
+fun transitiveDeps(vararg names: String, transitive: Boolean = true, platform: Boolean = false): NamedDependency =


I think that code is more java style way, by adding several overload methods for passing value for one argument, cause we don't have named parameters as it was in Java. This solution brings complicity for our API and will take some time to support documentation and improvements.
I think we need to add another type Platform, but I don't understand, whats impact we will be get from that.

Added support for use forma with external plugins. Also added platfor…

6fdb2a8

…m dependency type

ikarenkov mentioned this pull request Jan 14, 2021

Implement external plugins support API #35

Closed

ikarenkov commented Jan 14, 2021

View reviewed changes

stepango reviewed Jan 14, 2021

View reviewed changes

michaem reviewed Jan 15, 2021

View reviewed changes

Fix review

2b02356

stepango approved these changes Jan 22, 2021

View reviewed changes

ikarenkov merged commit b4b26fd into master Jan 23, 2021

ikarenkov deleted the custom_plugins_support branch January 23, 2021 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the ability to use eternal plugins with forma #75

Added the ability to use eternal plugins with forma #75

ikarenkov commented Jan 14, 2021

ikarenkov commented Jan 14, 2021

ikarenkov Jan 14, 2021

ikarenkov Jan 14, 2021

stepango Jan 14, 2021

ikarenkov Jan 14, 2021

michaem Jan 15, 2021

ikarenkov Jan 15, 2021

stepango Jan 14, 2021

stepango Jan 14, 2021

stepango Jan 14, 2021

ikarenkov Jan 14, 2021

ikarenkov Jan 14, 2021 •

edited

stepango Jan 19, 2021

stepango Jan 14, 2021 •

edited

ikarenkov Jan 14, 2021

stepango Jan 14, 2021 •

edited

ikarenkov Jan 14, 2021

michaem Jan 15, 2021 •

edited

stepango Jan 14, 2021

ikarenkov Jan 14, 2021

stepango Jan 14, 2021

ikarenkov Jan 14, 2021

michaem Jan 15, 2021

michaem Jan 15, 2021

michaem Jan 15, 2021

michaem Jan 15, 2021


		fun transitivePlatform(vararg names: String): NamedDependency = transitiveDeps(names = *names, transitive = true, platform = true)

		fun transitiveDeps(vararg names: String, transitive: Boolean = true, platform: Boolean = false): NamedDependency =

Added the ability to use eternal plugins with forma #75

Added the ability to use eternal plugins with forma #75

Conversation

ikarenkov commented Jan 14, 2021

ikarenkov commented Jan 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikarenkov Jan 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stepango Jan 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stepango Jan 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaem Jan 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikarenkov Jan 14, 2021 •

edited

stepango Jan 14, 2021 •

edited

stepango Jan 14, 2021 •

edited

michaem Jan 15, 2021 •

edited