You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by robertdale <gi...@git.apache.org> on 2016/08/18 19:32:26 UTC
[GitHub] tinkerpop pull request #385: TINKERPOP-1285 added multi-line line number sup...
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 <ro...@gmail.com>
Date: 2016-08-18T19:18:31Z
added multi-line line number support
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by okram <gi...@git.apache.org>.
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`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #385: TINKERPOP-1285 added multi-line line number sup...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/385
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by dkuppitz <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by robertdale <gi...@git.apache.org>.
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
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by robertdale <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by robertdale <gi...@git.apache.org>.
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
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by robertdale <gi...@git.apache.org>.
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>`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by spmallette <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by robertdale <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by spmallette <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #385: TINKERPOP-1285 added multi-line line number sup...
Posted by pluradj <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #385: TINKERPOP-1285 added multi-line line number sup...
Posted by spmallette <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/385
This was rebased on master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by spmallette <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by spmallette <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #385: TINKERPOP-1285 added multi-line line number support
Posted by spmallette <gi...@git.apache.org>.
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
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---