Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-1268

Improve script execution options for console

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Implemented
    • Affects Version/s: 3.1.2-incubating
    • Fix Version/s: 3.2.1
    • Component/s: console
    • Labels:

      Description

      Given the discussion here:

      https://pony-poc.apache.org/thread.html/Zhsn1tow4012xpm

      Implement the following:

      1. Deprecate support for ScriptExecutor
      2. Deprecate support of bin/gremlin.sh init.groovy
      3. Add support for bin/gremlin.sh -i init.groovy which does the same thing as (2) and does not exit the console on failure. That would allow a user to work with their console session up to the point of failure.
      4. Improve support for bin/gremlin.sh -e exec.groovy to no longer use ScriptExecutor and execute scripts directly in the console for automation purposes.
      5. Add some other options to control output to the console so that you could do bin/gremlin.sh -q -e exec.groovy which would execute in a quiet mode with no output, for example.

      The idea would be to try to implement this without breaking change.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/incubator-tinkerpop/pull/297

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/incubator-tinkerpop/pull/297
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user pluradj commented on a diff in the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/297#discussion_r61726988

          — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy —
          @@ -284,27 +315,98 @@ class Console

          { return STANDARD_RESULT_PROMPT }
          • private void initializeShellWithScript(final String initScriptFile) {
            + private void executeInShell(final List<String> scriptAndArgs) {
            + final String scriptFile = scriptAndArgs[0]
            try {
          • final File file = new File(initScriptFile)
          • file.eachLine { line ->
            + // check if this script comes with arguments. if so then set them up in an "args" bundle
            + if (scriptAndArgs.size() > 1) { + List<String> args = scriptAndArgs.subList(1, scriptAndArgs.size()) + groovy.execute("args = [\"" + args.join('\",\"') + "\"]") + }
              • End diff –

          I'm suggesting this
          ```
          if (scriptAndArgs.size() > 1)

          { List<String> args = scriptAndArgs.subList(1, scriptAndArgs.size()) groovy.execute("args = [\"" + args.join('\",\"') + "\"]") }

          else

          { groovy.execute("args = []") }

          ```

          so that this input groovy script
          ```
          println args
          ```

          doesn't fail like this
          ```
          /bin/gremlin.sh -e /tmp/foo.groovy
          Error in /tmp/foo.groovy at [1: println args] - No such property: args for class: groovysh_evaluate
          groovy.lang.MissingPropertyException: No such property: args for class: groovysh_evaluate
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user pluradj commented on a diff in the pull request: https://github.com/apache/incubator-tinkerpop/pull/297#discussion_r61726988 — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy — @@ -284,27 +315,98 @@ class Console { return STANDARD_RESULT_PROMPT } private void initializeShellWithScript(final String initScriptFile) { + private void executeInShell(final List<String> scriptAndArgs) { + final String scriptFile = scriptAndArgs [0] try { final File file = new File(initScriptFile) file.eachLine { line -> + // check if this script comes with arguments. if so then set them up in an "args" bundle + if (scriptAndArgs.size() > 1) { + List<String> args = scriptAndArgs.subList(1, scriptAndArgs.size()) + groovy.execute("args = [\"" + args.join('\",\"') + "\"]") + } End diff – I'm suggesting this ``` if (scriptAndArgs.size() > 1) { List<String> args = scriptAndArgs.subList(1, scriptAndArgs.size()) groovy.execute("args = [\"" + args.join('\",\"') + "\"]") } else { groovy.execute("args = []") } ``` so that this input groovy script ``` println args ``` doesn't fail like this ``` /bin/gremlin.sh -e /tmp/foo.groovy Error in /tmp/foo.groovy at [1: println args] - No such property: args for class: groovysh_evaluate groovy.lang.MissingPropertyException: No such property: args for class: groovysh_evaluate ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on a diff in the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/297#discussion_r61687353

          — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy —
          @@ -284,27 +315,98 @@ class Console

          { return STANDARD_RESULT_PROMPT }
          • private void initializeShellWithScript(final String initScriptFile) {
            + private void executeInShell(final List<String> scriptAndArgs) {
            + final String scriptFile = scriptAndArgs[0]
            try {
          • final File file = new File(initScriptFile)
          • file.eachLine { line ->
            + // check if this script comes with arguments. if so then set them up in an "args" bundle
            + if (scriptAndArgs.size() > 1) { + List<String> args = scriptAndArgs.subList(1, scriptAndArgs.size()) + groovy.execute("args = [\"" + args.join('\",\"') + "\"]") + }
              • End diff –

          That's done in [L166](https://github.com/apache/incubator-tinkerpop/pull/297/files#diff-94e9f2f26def3fb716d93349f670434aR166).

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on a diff in the pull request: https://github.com/apache/incubator-tinkerpop/pull/297#discussion_r61687353 — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy — @@ -284,27 +315,98 @@ class Console { return STANDARD_RESULT_PROMPT } private void initializeShellWithScript(final String initScriptFile) { + private void executeInShell(final List<String> scriptAndArgs) { + final String scriptFile = scriptAndArgs [0] try { final File file = new File(initScriptFile) file.eachLine { line -> + // check if this script comes with arguments. if so then set them up in an "args" bundle + if (scriptAndArgs.size() > 1) { + List<String> args = scriptAndArgs.subList(1, scriptAndArgs.size()) + groovy.execute("args = [\"" + args.join('\",\"') + "\"]") + } End diff – That's done in [L166] ( https://github.com/apache/incubator-tinkerpop/pull/297/files#diff-94e9f2f26def3fb716d93349f670434aR166 ).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user pluradj commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/297#issuecomment-216036513

          Looks good, Stephen. I reviewed the code, reviewed the docs, tested on Mac and Windows.

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user pluradj commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/297#issuecomment-216036513 Looks good, Stephen. I reviewed the code, reviewed the docs, tested on Mac and Windows. VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user pluradj commented on a diff in the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/297#discussion_r61685928

          — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy —
          @@ -284,27 +315,98 @@ class Console

          { return STANDARD_RESULT_PROMPT }
          • private void initializeShellWithScript(final String initScriptFile) {
            + private void executeInShell(final List<String> scriptAndArgs) {
            + final String scriptFile = scriptAndArgs[0]
            try {
          • final File file = new File(initScriptFile)
          • file.eachLine { line ->
            + // check if this script comes with arguments. if so then set them up in an "args" bundle
            + if (scriptAndArgs.size() > 1) { + List<String> args = scriptAndArgs.subList(1, scriptAndArgs.size()) + groovy.execute("args = [\"" + args.join('\",\"') + "\"]") + }
              • End diff –

          Should declare `args` as either `null` or `[]` here if none are passed in, otherwise a script allowing 0 or more args would hit an error for `args` not declared.

          Show
          githubbot ASF GitHub Bot added a comment - Github user pluradj commented on a diff in the pull request: https://github.com/apache/incubator-tinkerpop/pull/297#discussion_r61685928 — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy — @@ -284,27 +315,98 @@ class Console { return STANDARD_RESULT_PROMPT } private void initializeShellWithScript(final String initScriptFile) { + private void executeInShell(final List<String> scriptAndArgs) { + final String scriptFile = scriptAndArgs [0] try { final File file = new File(initScriptFile) file.eachLine { line -> + // check if this script comes with arguments. if so then set them up in an "args" bundle + if (scriptAndArgs.size() > 1) { + List<String> args = scriptAndArgs.subList(1, scriptAndArgs.size()) + groovy.execute("args = [\"" + args.join('\",\"') + "\"]") + } End diff – Should declare `args` as either `null` or `[]` here if none are passed in, otherwise a script allowing 0 or more args would hit an error for `args` not declared.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/297#issuecomment-215588978

          I mean to write this earlier....note that the addition of:

          ```xml
          <dependency>
          <groupId>commons-cli</groupId>
          <artifactId>commons-cli</artifactId>
          <version>1.2</version>
          </dependency>
          ```

          does affect the binary distribution for the console but it does not affect LICENSE/NOTICE. The NOTICE file included in commons-cli is boilerplate apache and therefore does not need to be included in our NOTICE.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/297#issuecomment-215588978 I mean to write this earlier....note that the addition of: ```xml <dependency> <groupId>commons-cli</groupId> <artifactId>commons-cli</artifactId> <version>1.2</version> </dependency> ``` does affect the binary distribution for the console but it does not affect LICENSE/NOTICE. The NOTICE file included in commons-cli is boilerplate apache and therefore does not need to be included in our NOTICE.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/297#issuecomment-215569655

          yeah - i thought about adding all the options to `getopts` and i did not like that idea especially since the console also supports long variants of each of the items:

          ```text
          bin/gremlin.sh -e script.groovy
          bin/gremlin.sh --execute=script.groovy
          ```

          was getting too complicated to try to maintain that.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/297#issuecomment-215569655 yeah - i thought about adding all the options to `getopts` and i did not like that idea especially since the console also supports long variants of each of the items: ```text bin/gremlin.sh -e script.groovy bin/gremlin.sh --execute=script.groovy ``` was getting too complicated to try to maintain that.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/297#issuecomment-215548386

          Looks good to me. I prefer this approach for option parsing:

          https://github.com/apache/incubator-tinkerpop/blob/master/docker/scripts/build.sh#L40-L50

          However, it requires that you handle all possible options and show a usage message if an option is unknown. That probably doesn't fit well in `gremlin.sh`, as the Java side is supposed to handle all "unknown" options and you likely don't want to keep the shell script and the Java side in sync whenever an option is added / removed.

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/297#issuecomment-215548386 Looks good to me. I prefer this approach for option parsing: https://github.com/apache/incubator-tinkerpop/blob/master/docker/scripts/build.sh#L40-L50 However, it requires that you handle all possible options and show a usage message if an option is unknown. That probably doesn't fit well in `gremlin.sh`, as the Java side is supposed to handle all "unknown" options and you likely don't want to keep the shell script and the Java side in sync whenever an option is added / removed. VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

          https://github.com/apache/incubator-tinkerpop/pull/297

          TINKERPOP-1268 Interactive and Executor Mode for Console

          This PR covers:

          https://issues.apache.org/jira/browse/TINKERPOP-1268
          https://issues.apache.org/jira/browse/TINKERPOP-1157
          https://issues.apache.org/jira/browse/TINKERPOP-1155
          https://issues.apache.org/jira/browse/TINKERPOP-1156

          I think that the console is working with a nice level of consistency now. We no longer have to warn people that the script they give as an init script is different than a they give to `-e` - it all executes in the same environment so no more discrepancies.

          You can see some examples in the documentation for how `-e` and `-i` are used. The shell script simplified a bit - @dkuppitz if you see a better way to deal with the `getopts` thing please let me know. I need to track `-l` in the gremlin.sh because we use it to dynamically set some log4j stuff and other debug natured things. `getopts` kinda feels like overkill but - it's working so.....

          I tested windows and it seemed to work.

          Please give it a shot and see if you can break it. I tried to test as many combinations as I could think of but i may have fell short somewhere.

          VOTE + 1

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-1268

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/incubator-tinkerpop/pull/297.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #297


          commit b1493b5b40434ff82c89790e3142819d569119ed
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-04-28T11:39:55Z

          Deprecate ScriptExecutor.

          commit b6853f052fc2efcf76f9d8e30bf95e6a22349fcc
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-04-28T17:46:02Z

          Implemented more options for Gremlin Console startup.

          Added -i for interactive mode and made -e execute in the Console rather than ScriptExecutor. Added switches to show/hide output and added a switch for "help". The existing method for -i still works as in "bin/gremlin.sh init.groovy"

          commit 1ed7043721214b04ebb278ae45503fc893f2b960
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-04-28T18:01:53Z

          Better exception messaging on failure of -e.

          commit 3ce732a3e7113ea707dc39755c2fa2f06a5682b3
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-04-28T18:08:44Z

          Updated changelog.

          commit 05be605cd3df603772b15aad751310ea2d4b4b89
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-04-28T18:39:21Z

          Allow override of verbosity to quiet when using -e.

          Only override if the verbosity is not explicitly set.

          commit 620f4787b3b6da01679e2f6b80aa3b1a9693f33a
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-04-28T18:59:37Z

          Use -i as this is now the preferred way of starting interactive mode

          commit 0e513cbc47f0885a0484b194ce81d31218dec6dd
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-04-28T19:00:04Z

          Fixed up sections for console interactive/execute modes.

          commit cd709ecb549d88d13dce0694c2d2d25de70baaf4
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-04-28T19:02:17Z

          Update upgrade docs for Console changes


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/incubator-tinkerpop/pull/297 TINKERPOP-1268 Interactive and Executor Mode for Console This PR covers: https://issues.apache.org/jira/browse/TINKERPOP-1268 https://issues.apache.org/jira/browse/TINKERPOP-1157 https://issues.apache.org/jira/browse/TINKERPOP-1155 https://issues.apache.org/jira/browse/TINKERPOP-1156 I think that the console is working with a nice level of consistency now. We no longer have to warn people that the script they give as an init script is different than a they give to `-e` - it all executes in the same environment so no more discrepancies. You can see some examples in the documentation for how `-e` and `-i` are used. The shell script simplified a bit - @dkuppitz if you see a better way to deal with the `getopts` thing please let me know. I need to track `-l` in the gremlin.sh because we use it to dynamically set some log4j stuff and other debug natured things. `getopts` kinda feels like overkill but - it's working so..... I tested windows and it seemed to work. Please give it a shot and see if you can break it. I tried to test as many combinations as I could think of but i may have fell short somewhere. VOTE + 1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-1268 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tinkerpop/pull/297.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #297 commit b1493b5b40434ff82c89790e3142819d569119ed Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-04-28T11:39:55Z Deprecate ScriptExecutor. commit b6853f052fc2efcf76f9d8e30bf95e6a22349fcc Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-04-28T17:46:02Z Implemented more options for Gremlin Console startup. Added -i for interactive mode and made -e execute in the Console rather than ScriptExecutor. Added switches to show/hide output and added a switch for "help". The existing method for -i still works as in "bin/gremlin.sh init.groovy" commit 1ed7043721214b04ebb278ae45503fc893f2b960 Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-04-28T18:01:53Z Better exception messaging on failure of -e. commit 3ce732a3e7113ea707dc39755c2fa2f06a5682b3 Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-04-28T18:08:44Z Updated changelog. commit 05be605cd3df603772b15aad751310ea2d4b4b89 Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-04-28T18:39:21Z Allow override of verbosity to quiet when using -e. Only override if the verbosity is not explicitly set. commit 620f4787b3b6da01679e2f6b80aa3b1a9693f33a Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-04-28T18:59:37Z Use -i as this is now the preferred way of starting interactive mode commit 0e513cbc47f0885a0484b194ce81d31218dec6dd Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-04-28T19:00:04Z Fixed up sections for console interactive/execute modes. commit cd709ecb549d88d13dce0694c2d2d25de70baaf4 Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-04-28T19:02:17Z Update upgrade docs for Console changes
          Hide
          spmallette stephen mallette added a comment -

          I have some basics for this working now. First, here's -i in action where a script error fails the init but the console stays open. Note how the error now shows the line number it stopped at:

          $ bin/gremlin.sh -i init.groovy
                   \,,,/
                   (o o)
          -----oOOo-(3)-oOOo-----
          plugin activated: tinkerpop.server
          plugin activated: tinkerpop.utilities
          plugin activated: tinkerpop.tinkergraph
          Invalid line in init.groovy at [3: z = y + xx] - No such property: xx for class: groovysh_evaluate
          gremlin> x
          ==>2
          

          Same script with -e goes right to error with no other output because of -q (for quiet output):

          $ bin/gremlin.sh -q -e init.groovy
          Invalid line in init.groovy at [3: z = y + xx] - No such property: xx for class: groovysh_evaluate
          

          I then get rid of the bad error in my script and just make it:

          x = 2
          y = x + 1
          z = y + x 
          println z
          

          and with -e we get:

          $ bin/gremlin.sh -q -e init.groovy
          5
          

          and with -i we get:

          $ bin/gremlin.sh -i init.groovy
                   \,,,/
                   (o o)
          -----oOOo-(3)-oOOo-----
          plugin activated: tinkerpop.server
          plugin activated: tinkerpop.utilities
          plugin activated: tinkerpop.tinkergraph
          5
          gremlin> x
          ==>2
          gremlin> y
          ==>3
          gremlin> z
          ==>5
          gremlin> 
          

          I also still support the args option on a script as we had before with -e so you can also do: bin/gremlin.sh -e script.groovy 1 2 3 4 and have 1,2,3,4 folded into a list of args that are accessible in the script. Ultimately I think we might want to get rid of that and simply support multiple scripts (like bin/gremlin.sh -e script1.groovy script2.groovy, ...), but that's a pretty big breaking change so perhaps that is for later discussion.

          Show
          spmallette stephen mallette added a comment - I have some basics for this working now. First, here's -i in action where a script error fails the init but the console stays open. Note how the error now shows the line number it stopped at: $ bin/gremlin.sh -i init.groovy \,,,/ (o o) -----oOOo-(3)-oOOo----- plugin activated: tinkerpop.server plugin activated: tinkerpop.utilities plugin activated: tinkerpop.tinkergraph Invalid line in init.groovy at [3: z = y + xx] - No such property: xx for class: groovysh_evaluate gremlin> x ==>2 Same script with -e goes right to error with no other output because of -q (for quiet output): $ bin/gremlin.sh -q -e init.groovy Invalid line in init.groovy at [3: z = y + xx] - No such property: xx for class: groovysh_evaluate I then get rid of the bad error in my script and just make it: x = 2 y = x + 1 z = y + x println z and with -e we get: $ bin/gremlin.sh -q -e init.groovy 5 and with -i we get: $ bin/gremlin.sh -i init.groovy \,,,/ (o o) -----oOOo-(3)-oOOo----- plugin activated: tinkerpop.server plugin activated: tinkerpop.utilities plugin activated: tinkerpop.tinkergraph 5 gremlin> x ==>2 gremlin> y ==>3 gremlin> z ==>5 gremlin> I also still support the args option on a script as we had before with -e so you can also do: bin/gremlin.sh -e script.groovy 1 2 3 4 and have 1,2,3,4 folded into a list of args that are accessible in the script. Ultimately I think we might want to get rid of that and simply support multiple scripts (like bin/gremlin.sh -e script1.groovy script2.groovy, ... ), but that's a pretty big breaking change so perhaps that is for later discussion.
          Hide
          spmallette stephen mallette added a comment -

          Last comment was wrong. Seems we still need both -e and -i given that script execution errors would terminate before :x and leave the console open for -e when you would probably prefer it to exit with error code for automation purposes. The alternative is true for -i as you would want the script execution to stop right at the failure so that you might debug what went wrong and have the state of the console to that point.

          Show
          spmallette stephen mallette added a comment - Last comment was wrong. Seems we still need both -e and -i given that script execution errors would terminate before :x and leave the console open for -e when you would probably prefer it to exit with error code for automation purposes. The alternative is true for -i as you would want the script execution to stop right at the failure so that you might debug what went wrong and have the state of the console to that point.
          Hide
          spmallette stephen mallette added a comment -

          I realized that there really is no need for -i because it is the same as -e. You can control the exit to the console with :x in the script and since I'm adding new controls for console output to the screen, you can show or hide what you want which really makes those two options the same in what they do.

          I did however mark this as a breaking change because if you had a script in the past that you executed with -e you would now need to alter your script and add some options for tuning the logs to hide some output so that it would behave in the same way as the ScriptExecutor did.

          Show
          spmallette stephen mallette added a comment - I realized that there really is no need for -i because it is the same as -e . You can control the exit to the console with :x in the script and since I'm adding new controls for console output to the screen, you can show or hide what you want which really makes those two options the same in what they do. I did however mark this as a breaking change because if you had a script in the past that you executed with -e you would now need to alter your script and add some options for tuning the logs to hide some output so that it would behave in the same way as the ScriptExecutor did.

            People

            • Assignee:
              spmallette stephen mallette
              Reporter:
              spmallette stephen mallette
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development