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
- links to
Activity
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
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.
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
```
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.
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`.
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.
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.
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.
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
```
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
- 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.
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
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
- 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.
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>`.
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
```
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.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/385
This was rebased on master.
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:
but includes line numbers. if we were to do something like this, i think we should consider following the groovy pattern.