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

Gremline console does not differentiate between multi-line and single-line input

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Done
    • Affects Version/s: 3.2.0-incubating
    • Fix Version/s: 3.2.2
    • Component/s: console
    • Labels:
      None

      Description

      When entering input, say into a script variable, that extends over multiple lines the gremlin console does not provide the user with any indication that they are in a multi-line input mode. Here is an example.
      Notice the 'gremline>' prompts are presented within the triple-quotes.

      gremlin> script = '''
      gremlin> line1_command
      gremlin> line2_command
      gremlin> '''
      

      Ideally we would like something like this, showing the user that they are still in multi-line input mode.

      gremlin> script = '''
      ... line1_command
      ... line2_command
      ... '''
      gremlin>
      

        Issue Links

          Activity

          Hide
          spmallette stephen mallette added a comment -

          Line numbers now clearly show when the console goes multi-line.

          Show
          spmallette stephen mallette added a comment - Line numbers now clearly show when the console goes multi-line.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/385

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

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          This was rebased on master.

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/385 This was rebased on master.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          feel free to move ahead with the rebase and i can then get this merged. I think that the zeros can go now that we have the dots which i like the more each time i see it. thanks - this was a really nice contribution.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/385 feel free to move ahead with the rebase and i can then get this merged. I think that the zeros can go now that we have the dots which i like the more each time i see it. thanks - this was a really nice contribution.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          @spmallette I can rebase on latest master. Just let me know. Do you think there is any point in keeping the leading zeroes?
          ```
          gremlin> 1+
          ......1> 2+
          ......2> 3+
          ......3> 4
          ```

          Alt prompt:
          ```
          g> 1+
          1> 2+
          2> 3+
          3> 4
          ==>10

          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/385 @spmallette I can rebase on latest master. Just let me know. Do you think there is any point in keeping the leading zeroes? ``` gremlin> 1+ ......1> 2+ ......2> 3+ ......3> 4 ``` Alt prompt: ``` g> 1+ 1> 2+ 2> 3+ 3> 4 ==>10 ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          OK. And when this is merged with the color preferences PR, it will be adjusted and left-filled with `.` for the input.prompt length, but no less than `000>`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/385 OK. And when this is merged with the color preferences PR, it will be adjusted and left-filled with `.` for the input.prompt length, but no less than `000>`.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tinkerpop/pull/385#discussion_r76053089

          — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy —
          @@ -191,7 +192,18 @@ class Console

          { groovy.setResultHook(handleResultShowNothing) }
          • private def handlePrompt = { interactive ? STANDARD_INPUT_PROMPT : "" }

            + private def handlePrompt = {
            + if (interactive) {
            + int lineNo = groovy.buffers.current().size()
            + if (lineNo > 0 ) {
            + return lineNo.toString().padLeft(STANDARD_INPUT_PROMPT_PAD, '0').padRight(STANDARD_INPUT_PROMPT.length()-2, ' ') + "> "

              • End diff –

          if it's easier to evaluate as merged, then i think we might want to complete the vote here for this work, as we already have three votes on the other PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/385#discussion_r76053089 — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy — @@ -191,7 +192,18 @@ class Console { groovy.setResultHook(handleResultShowNothing) } private def handlePrompt = { interactive ? STANDARD_INPUT_PROMPT : "" } + private def handlePrompt = { + if (interactive) { + int lineNo = groovy.buffers.current().size() + if (lineNo > 0 ) { + return lineNo.toString().padLeft(STANDARD_INPUT_PROMPT_PAD, '0').padRight(STANDARD_INPUT_PROMPT.length()-2, ' ') + "> " End diff – if it's easier to evaluate as merged, then i think we might want to complete the vote here for this work, as we already have three votes on the other PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          i prefer the `>` line up though - fwiw, of the above that i tinkered with I liked:

          ```text
          gremlin> 1 +
          ....001> 1 +
          ....002> 1 +
          ....003> 1
          ```

          the best

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/385 i prefer the `>` line up though - fwiw, of the above that i tinkered with I liked: ```text gremlin> 1 + ....001> 1 + ....002> 1 + ....003> 1 ``` the best
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tinkerpop/pull/385#discussion_r76052809

          — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy —
          @@ -191,7 +192,18 @@ class Console

          { groovy.setResultHook(handleResultShowNothing) }
          • private def handlePrompt = { interactive ? STANDARD_INPUT_PROMPT : "" }

            + private def handlePrompt = {
            + if (interactive) {
            + int lineNo = groovy.buffers.current().size()
            + if (lineNo > 0 ) {
            + return lineNo.toString().padLeft(STANDARD_INPUT_PROMPT_PAD, '0').padRight(STANDARD_INPUT_PROMPT.length()-2, ' ') + "> "

              • End diff –

          Yeah, it might be better to merge the 2 PRs together. `STANDARD_INPUT_PROMPT.length()` is replaced by the `input.prompt` preference from the colorization PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user pluradj commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/385#discussion_r76052809 — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy — @@ -191,7 +192,18 @@ class Console { groovy.setResultHook(handleResultShowNothing) } private def handlePrompt = { interactive ? STANDARD_INPUT_PROMPT : "" } + private def handlePrompt = { + if (interactive) { + int lineNo = groovy.buffers.current().size() + if (lineNo > 0 ) { + return lineNo.toString().padLeft(STANDARD_INPUT_PROMPT_PAD, '0').padRight(STANDARD_INPUT_PROMPT.length()-2, ' ') + "> " End diff – Yeah, it might be better to merge the 2 PRs together. `STANDARD_INPUT_PROMPT.length()` is replaced by the `input.prompt` preference from the colorization PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          I would only be against `===>` because that looks like the result prompt. I would probably lean towards `....001>`. Maybe:
          ```
          gremlin> 1 +
          001> 1 +
          002> 1 +
          003> 1
          ==>4
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/385 I would only be against `===>` because that looks like the result prompt. I would probably lean towards `....001>`. Maybe: ``` gremlin> 1 + 001> 1 + 002> 1 + 003> 1 ==>4 ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          I would think it should use the same color as the `input.prompt.color`. I'm not sure it needs to be configurable. Just wondering if there was a better presentation. For some reason, spaces look off to me, but I'll go with whatever people with stronger opinions are up for.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/385 I would think it should use the same color as the `input.prompt.color`. I'm not sure it needs to be configurable. Just wondering if there was a better presentation. For some reason, spaces look off to me, but I'll go with whatever people with stronger opinions are up for.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          Once the color preference support is added in, I can make this a preference. The line number can be '%n' so that it can be set with `:set multiline.prompt "%n >"`. Should it have its own color or just use input.prompt.color? So maybe wait until that merge happens first? Or I can merge this in with color preference PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/385 Once the color preference support is added in, I can make this a preference. The line number can be '%n' so that it can be set with `:set multiline.prompt "%n >"`. Should it have its own color or just use input.prompt.color? So maybe wait until that merge happens first? Or I can merge this in with color preference PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          I tested this out manually - works nicely. I don't really like the spaces between line number and `>`:

          ```text
          gremlin> 1 +
          001 > 1 +
          002 > 1 +
          003 > 1
          ==>4
          ```

          wonder if anyone else feels the same. i tried filling them with other things for fun to see if anything looked better:

          ```text
          gremlin> 1 +
          001>>>>> 1 +
          002>>>>> 1 +
          003>>>>> 1
          ==>4

          gremlin> 1 +
          001----> 1 +
          002----> 1 +
          003----> 1
          ==>4

          gremlin> 1 +
          001====> 1 +
          002====> 1 +
          003====> 1
          ==>4

          gremlin> 1 +
          ....001> 1 +
          ....002> 1 +
          ....003> 1
          ==>4
          ```

          Anyone feel the same? Otherwise:

          VOTE +1

          from me. Note that this PR could use a CHANGELOG entry and upgrade docs in the users section as i think it's a nice feature to announce. If @robertdale has time to amend this PR with that change, that would be cool, if not, whoever merges should tack that in.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/385 I tested this out manually - works nicely. I don't really like the spaces between line number and `>`: ```text gremlin> 1 + 001 > 1 + 002 > 1 + 003 > 1 ==>4 ``` wonder if anyone else feels the same. i tried filling them with other things for fun to see if anything looked better: ```text gremlin> 1 + 001>>>>> 1 + 002>>>>> 1 + 003>>>>> 1 ==>4 gremlin> 1 + 001----> 1 + 002----> 1 + 003----> 1 ==>4 gremlin> 1 + 001====> 1 + 002====> 1 + 003====> 1 ==>4 gremlin> 1 + ....001> 1 + ....002> 1 + ....003> 1 ==>4 ``` Anyone feel the same? Otherwise: VOTE +1 from me. Note that this PR could use a CHANGELOG entry and upgrade docs in the users section as i think it's a nice feature to announce. If @robertdale has time to amend this PR with that change, that would be cool, if not, whoever merges should tack that in.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          I agree with @dkuppitz . Lets make another ticket about deprecating and removing `IdentityRemovalStrategy`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/385 I agree with @dkuppitz . Lets make another ticket about deprecating and removing `IdentityRemovalStrategy`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          We should ask ourselves if `IdentityRemovalStrategy` is really as great as it should be. We need to chain dozens of `identity()`s to prove that `IdentityRemovalStrategy` makes the traversal execution faster. This looks way too artificial to me. Why would anybody end up with dozens of `identity()`s?

          IMHO we should either:

          • deprecate the strategy and remove it completely in a later release or
          • remove it from the default strategies and only run performance tests for more than 5 chained `identity()`s

          I think I'd prefer the former as I really can't see any real use cases where this strategy would be advantageous.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/385 We should ask ourselves if `IdentityRemovalStrategy` is really as great as it should be. We need to chain dozens of `identity()`s to prove that `IdentityRemovalStrategy` makes the traversal execution faster. This looks way too artificial to me. Why would anybody end up with dozens of `identity()`s? IMHO we should either: deprecate the strategy and remove it completely in a later release or remove it from the default strategies and only run performance tests for more than 5 chained `identity()`s I think I'd prefer the former as I really can't see any real use cases where this strategy would be advantageous.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          I lowered the threshold for passing on that test even lower in master yesterday after i saw travis has been failing over it lately. hopefully it is low enough now - if not @dkuppitz we may need to do something else with:

          ```text
          IdentityRemovalStrategyTest$PerformanceTest>TraversalStrategyPerformanceTest.shouldBeFaster:118 null
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/385 I lowered the threshold for passing on that test even lower in master yesterday after i saw travis has been failing over it lately. hopefully it is low enough now - if not @dkuppitz we may need to do something else with: ```text IdentityRemovalStrategyTest$PerformanceTest>TraversalStrategyPerformanceTest.shouldBeFaster:118 null ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/385

          This is failing due to some random error, not the code change.

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/385 This is failing due to some random error, not the code change.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user robertdale opened a pull request:

          https://github.com/apache/tinkerpop/pull/385

          TINKERPOP-1285 added multi-line line number support

          Looks like:
          ```
          gremlin>
          gremlin> 1 +
          001 2 +
          002 3 +
          003 x
          No such property: x for class: groovysh_evaluate
          Display stack trace? [yN]
          003 4
          ==>10
          ```
          Note that the line number remained the same for the correction.

          Example from the jira ticket:
          ```
          gremlin> script = """line1
          001 line2
          002 line3
          003 ...
          004 """
          ==>line1
          line2
          line3
          ...

          gremlin>
          ```

          Multi-line query:
          ```
          gremlin>
          gremlin> g.V().has(
          001 'name',
          002 'marko')
          ==>v[1]
          gremlin>
          ```

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

          $ git pull https://github.com/robertdale/tinkerpop TINKERPOP-1285

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

          https://github.com/apache/tinkerpop/pull/385.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 #385


          commit 96537e40844731ad7efd0e513fac15b13dae16d7
          Author: Robert Dale <robdale@gmail.com>
          Date: 2016-08-18T19:18:31Z

          added multi-line line number support


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user robertdale opened a pull request: https://github.com/apache/tinkerpop/pull/385 TINKERPOP-1285 added multi-line line number support Looks like: ``` gremlin> gremlin> 1 + 001 2 + 002 3 + 003 x No such property: x for class: groovysh_evaluate Display stack trace? [yN] 003 4 ==>10 ``` Note that the line number remained the same for the correction. Example from the jira ticket: ``` gremlin> script = """line1 001 line2 002 line3 003 ... 004 """ ==>line1 line2 line3 ... gremlin> ``` Multi-line query: ``` gremlin> gremlin> g.V().has( 001 'name', 002 'marko') ==>v [1] gremlin> ``` You can merge this pull request into a Git repository by running: $ git pull https://github.com/robertdale/tinkerpop TINKERPOP-1285 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/385.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 #385 commit 96537e40844731ad7efd0e513fac15b13dae16d7 Author: Robert Dale <robdale@gmail.com> Date: 2016-08-18T19:18:31Z added multi-line line number support
          Hide
          rocco.varela Rocco Varela added a comment -

          Following the groovy pattern sounds great.

          Show
          rocco.varela Rocco Varela added a comment - Following the groovy pattern sounds great.
          Hide
          spmallette stephen mallette added a comment -

          I don't really like this suggestion. I think it's important to keep the prompt for lines where you enter things. Note that groovysh keeps the prompt:

          groovy:000> script = """line1
          groovy:001> line2
          groovy:002> line3
          groovy:003> ...
          groovy:004> """
          ===> line1
          line2
          line3
          ...
          
          groovy:000> 
          

          but includes line numbers. if we were to do something like this, i think we should consider following the groovy pattern.

          Show
          spmallette stephen mallette added a comment - I don't really like this suggestion. I think it's important to keep the prompt for lines where you enter things. Note that groovysh keeps the prompt: groovy:000> script = """line1 groovy:001> line2 groovy:002> line3 groovy:003> ... groovy:004> """ ===> line1 line2 line3 ... groovy:000> but includes line numbers. if we were to do something like this, i think we should consider following the groovy pattern.

            People

            • Assignee:
              spmallette stephen mallette
              Reporter:
              rocco.varela Rocco Varela
            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development