#5 Adding DSL to define properties #9

mierzwid · 2019-01-02T16:08:28Z

user is able to specify
- default value
- if field is required
- validator
validation messages displayed in the props dialog

I propose the following DSL:

fork {
    config {
        cloneFiles()
        moveFiles(mapOf(
                "/com/company/aem/example" to "/{{projectGroup|substitute(\".\", \"/\")}}/{{projectName}}",
                "/Example" to "/{{projectLabel}}",
                "/example" to "/{{projectName}}"
        ))
        replaceContents(mapOf(
                "com.company.aem.example" to "{{projectGroup}}.{{projectName}}",
                "com.company.aem" to "{{projectGroup}}",
                "Example" to "{{projectLabel}}",
                "example" to "{{projectName}}"
        ))
    }
    defineProperties(mapOf(
            "aemSmbDomain" to {
                required()
                defaultValue = "Damian is cool"
                validator = {
                }
            },
            "aemInstanceLocalJarUrl" to {
                required()
                validator = {
                    if (!it.contains("@")) error("Oh, this is not an email!")
                }
            }
    ))
}

- user is able to specify -- default value -- if field is required -- validator - validation messages displayed in the props dialog

pun-ky · 2019-01-02T18:59:22Z

In validator dsl please change receiver instead of using 'it'

pun-ky · 2019-01-02T19:00:10Z

How about introducing new type of field, checkbox / boolean?

pun-ky · 2019-01-02T19:02:03Z

Maybe we should inverse logic because most of fields will be 'required', so that maybe you need to make it required by default an introduce instead 'optional' method

…asks

…Definition

src/main/kotlin/com/neva/gradle/fork/gui/PropertyDialogField.kt

pun-ky · 2019-01-02T18:52:10Z

src/main/kotlin/com/neva/gradle/fork/ForkExtension.kt

@@ -39,21 +41,33 @@ open class ForkExtension(val project: Project) {
  }

  fun inPlaceConfig(name: String, configurer: Config.() -> Unit) {
-    config(InPlaceConfig(project, name), configurer)
+    if (configExists(name)) {


I dislike extracting such trivial methods

src/main/kotlin/com/neva/gradle/fork/config/properties/PropertyDefinition.kt

mierzwid · 2019-01-04T10:11:42Z

PR updated, I will update README in next PR.
New DSL looks like this:

fork {
    properties(mapOf(
            "enableSmbClient" to {
                type = CHECKBOX
                defaultValue = "true"
            },
            "aemInstanceAuthorHttpUrl" to {
                defaultValue = "http://localhost:4502"
                validator = {
                    if (!value.startsWith("http", ignoreCase = true)) error("This is not an HTTP URL!")
                }
            },
            "aemInstanceAuthorJvmOpts" to {
                optional()
                defaultValue = "-server -Xmx1024m -XX:MaxPermSize=256M -Djava.awt.headless=true"
                validator = {
                    if (!value.startsWith("-")) error("This is not a JVM option!")
                }
            },
            "aemSmbUsername" to {
                defaultValue = System.getProperty("user.name")
            },
            "aemSftpPassword" to {
                type = PASSWORD
            },
            "projectGroup" to {
                defaultValue = "org.neva"
            }
    ))
    config {
        cloneFiles()
        moveFiles(mapOf(
                "/com/company/aem/example" to "/{{projectGroup|substitute(\".\", \"/\")}}/{{projectName}}",
                "/Example" to "/{{projectLabel}}",
                "/example" to "/{{projectName}}"
        ))
        replaceContents(mapOf(
                "com.company.aem.example" to "{{projectGroup}}.{{projectName}}",
                "com.company.aem" to "{{projectGroup}}",
                "Example" to "{{projectLabel}}",
                "example" to "{{projectName}}"
        ))
    }
}

pun-ky

very nice one!!! ;)

src/main/kotlin/com/neva/gradle/fork/config/Config.kt

pun-ky · 2019-01-04T10:39:29Z

src/main/kotlin/com/neva/gradle/fork/config/Config.kt

@@ -136,7 +142,7 @@ abstract class Config(val project: Project, val name: String) {
  }

  private fun promptValidate() {
-    val invalidProps = prompts.values.filter { !it.valid }.map { it.name }
+    val invalidProps = properties.filter { prop -> prop.validate().hasErrors() }.map { it.name }
    if (invalidProps.isNotEmpty()) {
      throw ForkException("Fork cannot be performed, because of missing properties: $invalidProps."


missing or invalid

val invalidProps = properties.filter(Property::isInvalid).map { it.name }

I mean, please correct message in exception

src/main/kotlin/com/neva/gradle/fork/config/InPlaceConfig.kt

pun-ky · 2019-01-04T10:40:57Z

src/main/kotlin/com/neva/gradle/fork/config/properties/PropertyDefinition.kt

+    if (name.isBlank()) throw PropertyException("Name of property definition cannot be blank!")
+  }
+
+  val PASSWORD = PropertyType.PASSWORD


what is the purpose of these vals?

Those values are used in DSL to simplify property type specification:
type = CHECKBOX
instead of
type = com.neva.gradle.fork.config.properties.PropertyType.CHECKBOX

ok, but even Gradle is not doing this that way (see Test task and setTestLogging accepting enum), importing enum will be not so bad and I am opting for this approach instead

src/main/kotlin/com/neva/gradle/fork/config/properties/PropertyDefinition.kt

pun-ky · 2019-01-04T10:42:55Z

src/main/kotlin/com/neva/gradle/fork/config/properties/PropertyDefinition.kt

+    errors.add(message)
+  }
+
+  fun hasErrors() = errors.isNotEmpty()


hasErrors -> passing

Do you mean:
val hasErrors = errors::isNotEmpty
?

I mean, name of method , nvm

src/main/kotlin/com/neva/gradle/fork/gui/PropertyDialogField.kt

pun-ky

how about introducing built-int / predefined validators for properties ending with Path or Url ? ;) which of course could be overidden / relaxed

szymon-owczarzak · 2019-01-04T12:22:38Z

src/main/kotlin/com/neva/gradle/fork/gui/PropertyDialogField.kt

+  }
+
+  private fun setPropertyValue() {
+    when (propField) {


can we use when as an expression here?

private fun setPropertyValue() = when (propField) { .... }

Only if I would add an empty else block. Since when is not returning any value I would leave it as it is.

Ok now i get it, we could also use sealed class and wrap all propField types in it but i think this is a bit different case. THX

src/main/kotlin/com/neva/gradle/fork/gui/PropertyDialogField.kt

mierzwid · 2019-01-04T13:03:32Z

I added two new types: PATH & URL and default validation for them. By default properties whose names end with "path" or "url" get those types. This can be overridden.

mierzwid · 2019-01-04T13:40:00Z

I already pushed updates to reflect new features in README.

pun-ky · 2019-01-07T09:13:16Z

README.md

@@ -129,6 +130,51 @@ Properties can be provided by (order makes precedence):

 4. Mixed approach.

+### Validating properties


defining section should be before validating ;)

OK, I changed naming

pun-ky

approved but code style may be still improved

pun-ky · 2019-01-07T09:14:17Z

README.md

+fork {
+    properties(mapOf(
+            "enableSmbClient" to {
+                type = CHECKBOX


I prefer PropertyType.CHECKBOX with import or introduce a method checkbox() in a same manner as optional() that mutes internal state

pun-ky · 2019-01-07T09:15:17Z

README.md

+                optional()
+                defaultValue = "-server -Xmx1024m -XX:MaxPermSize=256M -Djava.awt.headless=true"
+                validator = {
+                    if (!value.startsWith("-")) error("This is not a JVM option!")


if (value.split(" ").any{ !it.startsWith("-")) :)

pun-ky · 2019-01-07T09:16:12Z

src/main/kotlin/com/neva/gradle/fork/ForkExtension.kt

 import groovy.lang.Closure
 import org.gradle.api.Project
 import org.gradle.api.tasks.Input
 import org.gradle.util.ConfigureUtil

 open class ForkExtension(val project: Project) {

+  private val propertiesDefinitions = PropertyDefinitions()


only last part plural / propertyDefinitions

pun-ky · 2019-01-07T09:18:41Z

src/main/kotlin/com/neva/gradle/fork/ForkExtension.kt

@@ -23,23 +27,27 @@ open class ForkExtension(val project: Project) {
  }

  fun config(configurer: Config.() -> Unit) {
-    config(SourceTargetConfig(project, Config.NAME_DEFAULT), configurer)
+    config(SourceTargetConfig(project, propertiesDefinitions, Config.NAME_DEFAULT), configurer)


how about changing to

SourceTargetConfig(this, Config.NAME_DEFAULT)

the ForkExtension is already holding project, property definitions could be then public or internal. don't be afraid od passing these heavylooking objects. this will simplify code much

pun-ky · 2019-01-07T09:19:36Z

src/main/kotlin/com/neva/gradle/fork/config/Config.kt

@@ -14,13 +17,16 @@ import java.io.FileInputStream
 import java.io.FileOutputStream
 import java.util.*

-abstract class Config(val project: Project, val name: String) {
+abstract class Config(val project: Project, private val propertyDefinitions: PropertyDefinitions, val name: String) {


Config(ext: ForkExtension, val name: String)

pun-ky · 2019-01-07T09:25:39Z

src/main/kotlin/com/neva/gradle/fork/config/properties/PropertyDefinition.kt

+class PropertyDefinitions {
+  private val definitions = mutableMapOf<String, PropertyDefinition>()
+
+  fun configure(propertiesConfiguration: Map<String, PropertyDefinition.() -> Unit>) {


you are in a scope of Property (class name is scoping Property keyword) so that why no just configuration as an arg name

pun-ky · 2019-01-07T09:26:07Z

src/main/kotlin/com/neva/gradle/fork/config/properties/PropertyDefinition.kt

+    definitions += propertiesConfiguration.mapValues { PropertyDefinition(it.key).apply(it.value) }
+  }
+
+  fun getProperty(prompt: PropertyPrompt): Property {


however, you are right, in that case getProperty is ok.. :)

pun-ky · 2019-01-07T09:28:02Z

src/main/kotlin/com/neva/gradle/fork/config/properties/PropertyDefinition.kt

+    required = false
+  }
+
+  private fun calculateDefaultType() = when {


calculate is rather math related. I prefer here determine / examine etc

pun-ky · 2019-01-07T09:29:43Z

src/main/kotlin/com/neva/gradle/fork/gui/PropertyDialogField.kt

+import java.awt.Color
+import javax.swing.*
+
+class PropertyDialogField(private val property: Property, private val propField: JComponent, private val validationMessageLabel: JLabel, private val dialog: JDialog) {


please reformat that to
https://kotlinlang.org/docs/reference/coding-conventions.html#class-header-formatting

each field in constructor on separate line

pun-ky · 2019-01-07T09:30:23Z

src/main/kotlin/com/neva/gradle/fork/gui/PropertyDialogField.kt

+    get() = property.value
+
+  fun validateAndDisplayErrors(): Boolean {
+    setPropertyValue()


set indicates setter; please avoid setter like methods which are not setters actually

maybe better assign

mierzwid · 2019-01-11T11:54:53Z

I pushed changes addressing CR remarks. This version doesn't support GroovyDSL.

I have another branch that supports Groovy. However, validators don't work in Groovy and the overall solution is ugly hacking.

mierzwid · 2019-01-11T14:06:31Z

I finally added support for Groovy DSL, now DSL is exactly the same in Groovy and Kotlin:

properties {
            property("enableSomething") {
                checkbox(defaultValue = true)
            }
            property("someJvaOpts") {
                optional()
                text(defaultValue = "-server -Xmx1024m -XX:MaxPermSize=256M -Djava.awt.headless=true")
                validator {
                    if (!value.startsWith("-")) error("This is not a JVM option!")
                }
            }
            property("someUserName") {
                text(defaultValue = System.getProperty("user.name"))
            }
            property("projectGroup") {
                text(defaultValue = "org.neva")
            }
    }

pun-ky · 2019-01-11T16:19:22Z

README.md

+```
+
+#### Property definition
+Property definition can consists of:


blank line after header

pun-ky · 2019-01-11T16:19:44Z

README.md

+#### Property definition
+Property definition can consists of:
+* type specification: `type = TYPE_NAME`
+  * there are five types available: `TEXT` x(default one), `CHECKBOX` (representing boolean), `PASSWORD`, `PATH` & `URL`.


there are / not needed

pun-ky · 2019-01-11T16:22:03Z

src/main/kotlin/com/neva/gradle/fork/ForkExtension.kt

+    action.execute(this)
+  }
+
+  fun property(name: String, action: Action<in PropertyDefinition>) {


why in is here?

pun-ky · 2019-01-11T16:23:03Z

src/main/kotlin/com/neva/gradle/fork/ForkExtension.kt

+  }
+
+  fun property(name: String, action: Action<in PropertyDefinition>) {
+    val definition = project.objects.newInstance(PropertyDefinition::class.java, name)


you used new API introduced in 4.9 :) hope you know the purpose, teach me :)

pun-ky · 2019-01-11T16:24:23Z

src/main/kotlin/com/neva/gradle/fork/config/Config.kt

@@ -14,13 +16,18 @@ import java.io.FileInputStream
 import java.io.FileOutputStream
 import java.util.*

-abstract class Config(val project: Project, val name: String) {
+abstract class Config(private val forkExtension: ForkExtension, val name: String) {


just extension or ext. you will not use any other extensions so that fully qualified forkExtension is over-expressive for me (whole plugin is fork scoped :))

pun-ky · 2019-01-11T16:32:08Z

src/main/kotlin/com/neva/gradle/fork/config/properties/PropertyDefinition.kt

+package com.neva.gradle.fork.config.properties
+
+class PropertyDefinitions {
+  private val definitions = mutableMapOf<String, PropertyDefinition>()


mine and Cognifide's is matching each other, I have no problem with that :>

Intellij default + blank line around fields :D

pun-ky · 2019-01-11T16:33:35Z

src/main/kotlin/com/neva/gradle/fork/config/properties/PropertyDefinition.kt

+open class PropertyDefinition @Inject constructor(val name: String) {
+
+  init {
+    if (name.isBlank()) throw PropertyException("Name of property definition cannot be blank!")


follow Kotlin code style / if need { }

pun-ky · 2019-01-11T16:34:10Z

src/main/kotlin/com/neva/gradle/fork/config/properties/PropertyDefinition.kt

+  }
+
+  fun validator(validateAction: Action<in Validator>) {
+    validator = validateAction


why not this.validator = validator :)

I dislike repeating type in var/field name

pun-ky · 2019-01-11T16:38:07Z

src/main/kotlin/com/neva/gradle/fork/config/properties/PropertyDefinition.kt

+    this.defaultValue = defaultValue
+  }
+
+  fun url(defaultValue: String = "") {


consider implementing select option, it should not be hard?`

fun select(options: List<String>, defaultValue: String)` // when values <=> keys fun select(options: Map<String, String>, defaultValue: String)

pun-ky · 2019-01-11T16:38:54Z

src/main/kotlin/com/neva/gradle/fork/gui/PropertyDialogField.kt

+  private val propField: JComponent,
+  private val validationMessageLabel: JLabel,
+  private val dialog: JDialog
+) {


#5 Adding DSL to define properties

fd5372a

- user is able to specify -- default value -- if field is required -- validator - validation messages displayed in the props dialog

Damian Mierzwinski added 3 commits January 3, 2019 10:46

#5 Definitions of properties available both for fork and properties t…

4fcb622

…asks

#5 Repackaging properties

9ae9696

#5 Removing DSL classes and changing required to optional in Property…

58464bd

…Definition

pun-ky requested changes Jan 3, 2019

View reviewed changes

Damian Mierzwinski added 4 commits January 4, 2019 10:40

#5 Ignoring default values from templates

5abf246

#5 Introducing CHECKBOX property type

7abe9f3

#5 Changed receiver in validator to avoid using it

72a78f7

#5 Added default rules to infer property type based on its name

f7674c5

pun-ky reviewed Jan 4, 2019

View reviewed changes

szymon-owczarzak reviewed Jan 4, 2019

View reviewed changes

src/main/kotlin/com/neva/gradle/fork/gui/PropertyDialogField.kt Outdated Show resolved Hide resolved

szymon-owczarzak reviewed Jan 4, 2019

View reviewed changes

src/main/kotlin/com/neva/gradle/fork/gui/PropertyDialogField.kt Outdated Show resolved Hide resolved

szymon-owczarzak approved these changes Jan 4, 2019

View reviewed changes

#5 Applying CR remarks and adding default PATH and URL validation

39717a7

#5 Updating README to reflect properties definition

1d5d6a1

pun-ky reviewed Jan 7, 2019

View reviewed changes

pun-ky approved these changes Jan 7, 2019

View reviewed changes

pun-ky mentioned this pull request Jan 9, 2019

Encrypt password properties #10

Closed

#5 Addressing CR remarks; updating README.md

860533d

#5 support for groovy DSL

ec43ca7

pun-ky approved these changes Jan 11, 2019

View reviewed changes

pun-ky changed the base branch from master to develop January 14, 2019 13:10

pun-ky merged commit bc90d2e into neva-dev:develop Jan 14, 2019

		@@ -129,6 +130,51 @@ Properties can be provided by (order makes precedence):

		4. Mixed approach.

		### Validating properties

#5 Adding DSL to define properties #9

#5 Adding DSL to define properties #9

Conversation

mierzwid commented Jan 2, 2019

pun-ky commented Jan 2, 2019

pun-ky commented Jan 2, 2019

pun-ky commented Jan 2, 2019 • edited

Choose a reason for hiding this comment

mierzwid commented Jan 4, 2019

pun-ky left a 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

pun-ky Jan 7, 2019 • 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

pun-ky left a comment • edited

Choose a reason for hiding this comment

szymon-owczarzak Jan 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mierzwid commented Jan 4, 2019

mierzwid commented Jan 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pun-ky left a 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

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

mierzwid commented Jan 11, 2019

mierzwid commented Jan 11, 2019

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

Choose a reason for hiding this comment

pun-ky commented Jan 2, 2019 •

edited

pun-ky Jan 7, 2019 •

edited

pun-ky left a comment •

edited

szymon-owczarzak Jan 4, 2019 •

edited

mierzwid commented Jan 4, 2019 •

edited