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

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

Details

    • Improvement
    • Status: Closed
    • Minor
    • Resolution: Done
    • 3.2.0-incubating
    • 3.2.2
    • console
    • 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>
      

      Attachments

        Issue Links

          Activity

            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.

            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.
            rocco.varela Rocco Varela added a comment -

            Following the groovy pattern sounds great.

            rocco.varela Rocco Varela added a comment - Following the groovy pattern sounds great.
            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


            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
            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.

            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.
            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
            ```

            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 ```
            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.

            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.
            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`.

            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`.
            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.

            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.
            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.

            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.
            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.

            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.
            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
            ```

            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 ```
            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.

            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.
            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

            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
            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.

            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.
            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>`.

            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>`.
            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

            ```

            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 ```
            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.

            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.
            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.

            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.
            githubbot ASF GitHub Bot added a comment -

            Github user asfgit closed the pull request at:

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

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

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

            spmallette Stephen Mallette added a comment - Line numbers now clearly show when the console goes multi-line.

            People

              spmallette Stephen Mallette
              rocco.varela Rocco Varela
              Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: