Swift Package Manager plugin for Kotlin by PaGr0m · Pull Request #1 · PaGr0m/kotlin

PaGr0m · 2021-01-11T15:18:56Z

No description provided.

zoldater · 2021-01-18T14:51:04Z

+ * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
+ */
+
+@file:Suppress("PackageDirectoryMismatch") // Old package for compatibility


Remove this annotation and move sources to org.jetbrains.kotlin.gradle.plugin.spm package

zoldater · 2021-01-18T14:53:02Z

+    // TODO: List<String> or String
+    @Input
+    @Optional
+    var defaultLocalization: List<String>? = null


Add KDoc with references to corresponding SPM configuration parameters (for another properties too)

zoldater · 2021-01-18T14:56:59Z

+//            }
+        }
+
+        class SystemLibrary(


Make SystemLibrary implementing org.gradle.api.Named

zoldater · 2021-01-18T15:03:06Z

+            @Optional
+            var resources: List<Resource>? = null
+
+            fun dependencies(configure: TargetDependencies.() -> Unit) {


Functions with Kotlin lambdas parameters are visible only from kts scripts. To provide corresponding functionality to groovy scripts implement the overloading (see CocoapodsExtension.CocoapodsDependency#git(java.lang.String, groovy.lang.Closure<?>))

zoldater · 2021-01-18T15:04:01Z

+                dependenciesProduct = dependency.products
+            }
+
+            fun exclude(configure: ExcludePath.() -> Unit) {


The same as for dependencies

zoldater · 2021-01-18T15:04:07Z

+                excludePaths = exclude.paths
+            }
+
+            fun sources(configure: SourceFile.() -> Unit) {


The same as for dependencies

zoldater · 2021-01-18T15:04:12Z

+                sourceFiles = sources.files
+            }
+
+            fun resources(configure: Resource.() -> Unit) {


The same as for dependencies

zoldater · 2021-01-18T15:06:09Z

+//            fun swiftSettings() {}
+//            fun linkerSettings() {}
+
+            class Resource() {


It worth moving nested declarations into separate files because there are no private visibility restrictions.

zoldater · 2021-01-18T15:11:52Z

+                @Optional
+                var localization: Localization? = null
+
+                constructor(path: String) : this() {


Try to replace multiple constructors with default values or introduce builder for creating instances.

zoldater · 2021-01-18T15:15:44Z

+        platforms = supportedPlatform.platforms
+    }
+
+    fun products(configure: Products.() -> Unit) {


overloading with overloading groovy.lang.Closure (see CocoapodsExtension.CocoapodsDependency#git(java.lang.String, groovy.lang.Closure))

zoldater · 2021-01-18T15:16:18Z

+    // TODO: cLanguageStandard: CLanguageStandard? = nil,
+    // TODO: cxxLanguageStandard: CXXLanguageStandard? = nil
+
+    fun platforms(configure: SupportedPlatform.() -> Unit) {


zoldater · 2021-01-18T15:16:44Z

+        productsExecutables = product.executables
+    }
+
+    fun dependencies(configure: Dependencies.() -> Unit) {


zoldater · 2021-01-18T15:17:01Z

+        dependencies = dependency.dependencies
+    }
+
+    fun targets(configure: Targets.() -> Unit) {


zoldater · 2021-01-18T15:22:22Z

+    }
+
+    // TODO: add fun with another params (supportedPlatform)
+    class SupportedPlatform() {


Please, try to rethink this part. Supported is just the attribute of platform, but here SupportedPlatform is responsible for holding info like container and could be changed by its function calls.

Take a look at org.jetbrains.kotlin.gradle.plugin.KotlinPlatformType and its usages (for example, org/jetbrains/kotlin/gradle/plugin/mpp/KotlinMultiplatformPlugin.kt:302)

zoldater · 2021-01-18T15:29:58Z

+            platforms.add(platform)
+        }
+
+        enum class PlatformType {


MPP plugin contains proper API for platforms, re-use it (see org.jetbrains.kotlin.gradle.plugin.KotlinTarget#getPlatformType)

zoldater · 2021-01-18T15:31:44Z

+    }
+
+    class Products {
+        var libraries = mutableListOf<ProductType.Library>()


Use polymorphism

zoldater · 2021-01-18T15:33:40Z

+        }
+
+        sealed class ProductType {
+            class Executable(@Input val name: String) {


Nesting is not required here but significantly complicates code-readability. Make sources more flat, please.

zoldater · 2021-01-18T15:34:11Z

+                }
+            }
+
+            class Library(@Input val name: String) {


org.gradle.api.Named

zoldater · 2021-01-18T15:36:10Z

+    class Dependencies {
+        var dependencies = mutableListOf<Package>()
+
+        fun `package`(url: String, version: String) {


Manual overloading and `` quotes are not required. Use default parameters with @JvmOverloads annotation

zoldater · 2021-01-18T15:39:41Z

+        }
+
+        /** TODO
+         * Все target оказались одинаковыми по сигнатурам,


А обрабатывать в плагине их тоже нужно одинаково? Думаю, что нет, поэтому важно дизайнить API с заделом на будущее.

zoldater · 2021-01-18T15:41:40Z

+    }
+
+    class Targets {
+        // TODO: так и оставить как 3 списка?


Полиморфизм + 1 список. И классы, хранящие коллекцию каких-то сущностей, для удобства называют с суффиксом Container.

zoldater · 2021-01-18T15:42:23Z

+        var libraries = mutableListOf<SystemLibrary>()
+        var binaries = mutableListOf<BinaryTarget>()
+
+        fun target(name: String, configure: Target.() -> Unit) {


Don't forget about Groovy overloadings

zoldater

TODO для следующей итерации код-ревью:

Распределить сущности по файлам и пакетам в разумных масштабах
Посмотреть на некоторые сущности - таргеты, платформы и т.д., которые есть в MPP-плагине и могут быть переиспользованы. За конкретными примерами пиши в личку, подскажу.
Не самые удачные абстракции с классами-контейнерами, которые аккумулируют в себе дофига логики. Скорее всего большая часть из них может быть заменена на проперть с project.container(<YourClassHere>::class.java) и обработку
Если есть набор сущностей, похожих по сигнатурам, но отличающихся по какому-либо признаку, даже если сейчас этот признак не имеет значения, используем полиморфизм.
Избегаем копи-пасты, излишней мутабельности и велосипедостроения.

Rework container for entities.

PaGr0m · 2021-01-31T21:16:58Z

+/**
+ * TODO: comment
+ *
+ * @see [Product](https://docs.swift.org/package-manager/PackageDescription/PackageDescription.html#product)


Наверное стоит оставить только один источник (первый с оф. документации, второй с github), Но проблема в том, что в них разная информации (не актуальная)

Пока это для нашего с тобой пользования, можешь не запариваться

PaGr0m · 2021-01-31T21:19:24Z

+    val platforms = mutableListOf<SupportedPlatform>()
+
+    fun macOS(version: String) {
+        val platform = SupportedPlatform(Family.OSX, version)


Такое использование Family подойдет? Или имелось в виду, что стоит полностью переработать SupportedPlatforms ?

Такая реализация вполне хороша

PaGr0m · 2021-01-31T21:20:43Z

+
+package org.jetbrains.kotlin.gradle.plugin.spm.utils
+
+abstract class StringUnaryPlusContainer {


Хорошая идея? Или не стоит так выносить логику?

Идея хорошая, но лучше закинуть в companion targetBlock'а и снизить видимость

PaGr0m · 2021-01-31T21:24:24Z

+    class Executable(name: String) : AbstractProduct(name)
+
+    class Library(name: String, private val type: LibraryType) : AbstractProduct(name) {
+        enum class LibraryType {


Выбрал вариант с enum, как по мне, удобнее чем просто String. Но заставляет импортировать enum в gradle. Насколько это ок?
https://github.com/PaGr0m/spm-sample/blob/c50f8ea6d2cfc3e4e981c24788ce0d9208b97cbd/build.gradle.kts#L1

Это лучше, чем сырая строка, но для up-to-date проверок нужна будет динамическая строковая проперть

PaGr0m · 2021-01-31T21:25:39Z

+    @Input
+    lateinit var name: String
+
+    private val platformsContainer = project.container(SupportedPlatformBlock.SupportedPlatform::class.java)


Может тоже стоит создать интерфейс, как и с другими контейнерами? Хоть тут он особо не нужен, так как реализация одна. Но читаемость улучшит

Не нужно

PaGr0m · 2021-01-31T21:30:39Z

Пример DSL синтаксиса: build.gradle.kts example

zoldater · 2021-02-04T12:43:13Z

+    }
+
+    data class SupportedPlatform(
+        @Input val type: Family,


Family - элемент перечисления, как Input его использовать нельзя. Пометь как Internal и добавь стринговую проперть c type.name как Input

zoldater · 2021-02-04T12:46:38Z

+    }
+
+    abstract class AbstractTarget(private val name: String) : Target {
+        override fun getName(): String = name


zoldater · 2021-02-04T12:57:53Z

+        private val name: String,
+        val `package`: String,
+        val condition: String? = null
+    ) : TargetDependency {


TargetDependency тоже сюда можно перенести

PaGr0m added 2 commits December 17, 2020 19:44

Plugin initial

4a84526

Add some DSL (SPM extension)

cda6533

PaGr0m requested a review from zoldater January 11, 2021 15:25

zoldater reviewed Jan 18, 2021

View reviewed changes

zoldater requested changes Jan 18, 2021

View reviewed changes

Add polymorphism for Target, Product, Dependency.

b30b798

Rework container for entities.

PaGr0m commented Jan 31, 2021

View reviewed changes

zoldater reviewed Feb 4, 2021

View reviewed changes


		package org.jetbrains.kotlin.gradle.plugin.spm.utils

		abstract class StringUnaryPlusContainer {

Conversation

PaGr0m commented Jan 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoldater Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoldater Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoldater Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoldater left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zoldater Jan 18, 2021 •

edited

Loading

zoldater Jan 18, 2021 •

edited

Loading

zoldater Jan 18, 2021 •

edited

Loading