PT-429: input scripting #121

tasomaniac · 2017-01-31T17:07:17Z

Problem

Currently, user has to create a new task extending from Input to have input scripting. It doesn't really look good and we don't have much control over that task since we don't create it ourselves.

Solution

This is a proposal of a possible solution. I would like to get opinions and structure possible follow-ups for other tasks.

A NamedDomainObjectContainer is introduced in the extension. This is what productFlavors use in Android. This will allow users to have scripts like below

scripts {
    autoLogin {
        execute {
            2.times {
                text 'bob'
                enter()
            }
            enter()
        }
    }
}

This will then create an InputSpec object which will holds a Closure to be lazily executed in runtime of the task.

For backward compatibility, Closure script in Input task is not removed. But instead it now uses InputSpec as an inner field. script is converted to write-only property to keep backward compatibility. When called, prints a warning with a link to the appropriate section in README.

Deprecation is even visible to users in their build.gradle files

Have an named extension in the command extension to support DSL like scripting for inputs. Put them into a group named `add script`

Also move a method in the spec to make more sense.

tasomaniac · 2017-01-31T17:08:22Z

plugin/src/main/groovy/com/novoda/gradle/command/AndroidCommandPlugin.groovy

+    private configureInputScripts(AndroidCommandPluginExtension extension, Project project) {
+        extension.scripts.all { input ->
+            project.tasks.create(input.name, Input) {
+                group = 'adb script'


I decided to have a brand new group called adb script. Suggestions are welcome.

mr-archano · 2017-02-02T12:57:28Z

plugin/src/main/groovy/com/novoda/gradle/command/AndroidCommandPluginExtension.groovy

@@ -83,6 +86,14 @@ public class AndroidCommandPluginExtension {
        monkey
    }

+    void script(Action<NamedDomainObjectContainer<InputSpec>> action) {


I'm confused by this method: why do you need it? I'd expect you would expose the container itself (look at BPP for instance). This way the user will be able to create all the scripts he needs and then you can (lazily) parse all the InputSpec in there to create the Input tasks you need.

That line in BPP adds an extension to the project, right? I want to add the same to the command extension we already have.
Should I do something like project.android.command.extensions.add()?

And I didn't understand how it will help being lazy. All the code in the extension closure runs immediately with any gradle task in the configuration phase, right?

We might want to take a look at this tomorrow together, but beware the code executed in each entry of the container will be affecting only the InputSpec that is perfectly fine, because it's just about collecting the commands to be used later in the Input task

Yep thats perfectly fine. I think it is 2 different ways of achieving the same thing. Let's discuss tomorrow and decide on a solution.

Oh, and about adding the container you don't need to do anything special, just make the scripts field public in the extension (in BPP we want to add it to the project we have no extension there)

I'm not really happy with evaluating the scripts early. That limits their use quite a bit since we lose all kinds of runtime evaluations. Or am I missing something?

mr-archano · 2017-02-02T13:03:48Z

plugin/src/main/groovy/com/novoda/gradle/command/Input.groovy

-        def command = ["shell", "input"]
-        command.addAll(values)
-        runCommand(command)
+    @Deprecated


This method has been just added and already deprecated: what's the point of it? 😕
If you want to introduce this change without breaking any public API you should keep all the public methods deleted, mark them as @Deprecated and delegate to the InputSpec instead. Other choice will be to annotate the script field as @Delegate and let Groovy do its magic, but then the users won't have any notion of deprecation.

I don't want to deprecate the field script because now it is a proper input that I use. It is now used when the task is created in the plugin.

Before we had Closure script. With gradle magic, when the user creates a new task with type Input, they could use script like below (which looks like a method call)

This method is now introduced to support that the old behavior.

task autoLogin(type: com.novoda.gradle.command.Input) { script { } }

I didn't want to keep all the methods because that would cause a lot of duplication. I know that it is not ideal but with this we are 100% backward compatible.

I will look into @Delegate. I guess with that I can just remove this method. I would have to remove deprecation but it is not really super visible anyways.

You're talking from the perspective of the sample we provide, not the real public API: if you remove public methods you're introducing breaking changes, right? Duplication in this case is not correct, because you will not duplicate the implementation, will just be tedious to make those method to use delegation instead. I feel we can bare with that as long as we have a proper strategy to move forward :)

That's very true :)
Then Delegate seems to be a logical way to do it.

mr-archano · 2017-02-02T13:12:30Z

plugin/src/main/groovy/com/novoda/gradle/command/AndroidCommandPluginExtension.groovy

@@ -83,6 +86,14 @@ public class AndroidCommandPluginExtension {
        monkey
    }

+    void script(Action<NamedDomainObjectContainer<InputSpec>> action) {
+        action.execute(scripts)


Isn't because of this that you can't have lazy initialisation of commands?

I guess so. But I kinda need to execute the action to retrieve the name to create the task.

In the following example, I need to get the name autoLogin in advance to create the task with that name but still need to run the rest lazily. How would I do that?

By doing action.execute, the script container gets an InputSpec object with a name.

autoLogin { 2.times { text 'bob' enter() } enter() }

You can postpone the creation of the task waiting for the end of the configuration phase, after which the scripts container will be filled with all the entries. we do something similar in the other plugin :)

That makes sense. 👍 Should totally do that.

Keep the input scripting evaluation lazy by introducing another closure in the middle.

tasomaniac · 2018-01-08T16:22:32Z

plugin/src/main/groovy/com/novoda/gradle/command/Input.groovy

+     * https://github.com/novoda/gradle-android-command-plugin#input-scripting
+     */
+    @Deprecated
+    void setScript(Closure script) {


Looks like a new method deprecated but it really is the existing property being converted to write-only property and deprecated. Here is what I want to achieve.

Existing implementations will mark it as deprecated.

tasomaniac · 2018-01-08T16:28:18Z

README.md

+```groovy
+inputScripts {
+    autoLogin {
+        inputScript {


Here is how the new NamedDomainObjectContainer is used. Unfortunately I had to create another inputScript property in the middle to make the execution of the Closure lazy.

When I remove it and have the closure in one level up (like below), configure method of NamedDomainObjectContainer just executes the whole closure.

inputScripts { autoLogin { 2.times { text 'bob' enter() } enter() } }

…w `execute`

tasomaniac · 2018-01-25T21:24:26Z

Since there are no behavior changes with this change, I will go ahead and merge this.

Said Tahsin Dane added 3 commits January 31, 2017 17:29

Move all input related configuration to InputSpec class

e82dcaf

Have an named extension in the command extension to support DSL like scripting for inputs. Put them into a group named `add script`

Modify autoLogin to use new scripting style

4c9c08b

Add instructions about scripting in the README.

2f218ed

Also move a method in the spec to make more sense.

tasomaniac commented Jan 31, 2017

View reviewed changes

Fix readme link

8272bfd

mr-archano requested a review from devisnik February 2, 2017 11:30

mr-archano suggested changes Feb 2, 2017

View reviewed changes

Keep the changes minimal by not moving the methods from Input task.

6ee04a7

Keep the input scripting evaluation lazy by introducing another closure in the middle.

tasomaniac force-pushed the PT-429/input-scripting branch from 5a06e97 to 6ee04a7 Compare January 8, 2018 16:21

tasomaniac commented Jan 8, 2018

View reviewed changes

Update readme according to recent changes

18560d2

tasomaniac commented Jan 8, 2018

View reviewed changes

Said Tahsin Dane added 2 commits January 9, 2018 11:03

Update names, inputScripts is simply scripts, inputScript is no…

dc63af1

…w `execute`

Add another example

1c86533

mr-archano approved these changes Jan 9, 2018

View reviewed changes

Rename classes. Instead of Spec prefer Extension

0eb4475

tasomaniac merged commit 38f4d80 into develop Jan 25, 2018

tasomaniac deleted the PT-429/input-scripting branch January 25, 2018 21:24

tasomaniac added this to the v2.0 milestone Jan 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PT-429: input scripting #121

PT-429: input scripting #121

tasomaniac commented Jan 31, 2017 •

edited

Loading

tasomaniac Jan 31, 2017

mr-archano Feb 2, 2017

tasomaniac Feb 2, 2017 •

edited

Loading

mr-archano Feb 2, 2017

tasomaniac Feb 2, 2017

mr-archano Feb 2, 2017

devisnik Feb 2, 2017

mr-archano Feb 2, 2017

tasomaniac Feb 2, 2017

mr-archano Feb 2, 2017 •

edited

Loading

tasomaniac Feb 2, 2017

mr-archano Feb 2, 2017

tasomaniac Feb 2, 2017

mr-archano Feb 2, 2017

tasomaniac Feb 2, 2017

tasomaniac Jan 8, 2018

tasomaniac Jan 8, 2018

tasomaniac commented Jan 25, 2018

PT-429: input scripting #121

PT-429: input scripting #121

Conversation

tasomaniac commented Jan 31, 2017 • edited Loading

Problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tasomaniac Feb 2, 2017 • edited Loading

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

mr-archano Feb 2, 2017 • edited Loading

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

tasomaniac commented Jan 25, 2018

tasomaniac commented Jan 31, 2017 •

edited

Loading

tasomaniac Feb 2, 2017 •

edited

Loading

mr-archano Feb 2, 2017 •

edited

Loading