Description
In order to increase the readability of the output from the gremlin shell, it would be nice to have some coloring in the output. For example coloring the edges in the textual output.
Attachments
Issue Links
- links to
Activity
GitHub user robertdale opened a pull request:
https://github.com/apache/tinkerpop/pull/384
TINKERPOP-1037 Made life more colorful
Stephen did all the hard work.
- Added option -C to disable ANSI colors just in case.
- Colorized the gremlin ascii art.
- Added bold red to some, but probably not all, error messages
- Snuck in the help text for
TINKERPOP-1246
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/robertdale/tinkerpop TINKERPOP-1037
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/384.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 #384
commit ef518fbfef1910a73183abeed0bee90ba38e9a1f
Author: Robert Dale <robdale@gmail.com>
Date: 2016-08-18T17:34:04Z
made life more colorful
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/384
that looks like all the same code i've written in the past...............................
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
Like I said, you did all the hard work. I just added color. Try this in your console:
gremlin> "@|bold,green Hello |@ "
==>Hello
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
Screenshot: ![screenshot_2016-08-18_13-59-58](https://cloud.githubusercontent.com/assets/122206/17784872/4286944a-654c-11e6-8813-025bc3974601.png)
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/384
Hm, now the console has some colors, that's nice, but `TINKERPOP-1037` is actually about output coloring (which means to me, that results should have a bit of syntax coloring, right?). What do we need to do in order to have colored results (e.g. Strings in red, numbers in bllue, etc. et.c)?
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/384
Whoa. This is really cool. However, I think we should have a `.gremlin_preferences` style file where we do this:
```
gremlin.console.errorMessage=red
gremlin.console.prompt=white
gremlin.console.character=green
etc.
```
Then people can tweak to their liking and if they don't like it, they can go NOT have that file and it will be as it was before.
Github user alaam commented on the issue:
https://github.com/apache/tinkerpop/pull/384
+1 for having this be a preferences file
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
There are some things that are out of our control because they are inherited from groovy.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
Would you really want a separate preferences (properties) file or rather reuse the existing preferences mechanism (:set gremlin.console.errorMessage red) ?
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/384
+1 for use of `:set`
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/384
+1 for `:set`. Look at our other `sets` to get a consistent naming convention, please.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
![screenshot_2016-08-18_18-54-19](https://cloud.githubusercontent.com/assets/122206/17793465/77a45b16-6575-11e6-99ce-bef4bb1471af.png)
-
- Color codes:
- BLACK
- RED
- GREEN
- YELLOW
- BLUE
- MAGENTA
- CYAN
- WHITE
-
-
- Background Colors
-
- BG_BLACK
- BG_RED
- BG_GREEN
- BG_YELLOW
- BG_BLUE
- BG_MAGENTA
- BG_CYAN
- BG_WHITE
-
-
- Attributes
-
- RESET
- INTENSITY_BOLD
- INTENSITY_FAINT
- ITALIC
- UNDERLINE
- BLINK_SLOW
- BLINK_FAST
- BLINK_OFF
- NEGATIVE_ON
- NEGATIVE_OFF
- CONCEAL_ON
- CONCEAL_OFF
- UNDERLINE_DOUBLE
- UNDERLINE_OFF
- BOLD
- FAINT
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/384
nice @robertdale - a final request from my end would be documentation. perhaps a word or two added to the docs about these settings. I guess the reference docs might be the best place for that:
then maybe an announcement of the feature in upgrade docs for users:
and if you want to really be complete an entry in changelog:
@dkuppitz brought up the full scope of the ticket, but i think we can save the bigger picture for some other time. at least the pattern for colors is in place and working.
I still haven't tested this out myself or reviewed the code just yet to see what you did differently than me to get this to work. My mind is still kinda reeling from that. :disappointed:
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
I think a mockup of the desired coloring effect would be nice to have. Currently elements just have toString called on them. I added a vertex.color and edge.color for demonstration purposes. Here's what it would look like. Let me know if that's worth committing.
![screenshot_2016-08-18_21-00-22](https://cloud.githubusercontent.com/assets/122206/17795525/12511530-6587-11e6-8427-71d6e033a026.png)
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/384
Awesome! I don't like the color choice, but it's good to see that it works. How did you do it? Did you overwrite the respective `toString()` methods?
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
I'm definitely not the right person to pick a color scheme. Open for suggestions or we can figure it out later. Committed the vertex, edge, path coloring so you can see.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/384
Oh nice, I like that.
VOTE: +1
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
Attached groovy script that prints the color palette. usage: groovy jansi-colors.txt
[jansi-colors.txt](https://github.com/apache/tinkerpop/files/427016/jansi-colors.txt)
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/384
Thoughts:
1. I think there is a bit of an overdose of options here. Keep it simple.
2. We have to realize that not everyone has a black background as their terminal. I use a light brown. Some people use white. Thus, I don't think the default should be to use color. You can `:set` colors if you want and perhaps we even have "standard themes" – `:set black-background-standard` (or whatever the naming convention that we are using for other `:set` commands so we are consistent in our naming of parameters.
If you guys want to go with edge/vertex/etc. being able to be colored, please don't have that be part of the "standard templates." Keep things clean and simple and not a "disco club" coming at you.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
Disco disco! ANSI is enabled by default. This is actually not a change. It was this way before this PR. No color scheme by default. If you want colors, you have to set them yourself. Added colorizing for String, Number, and values within Iterables.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/384
Great. What do you think about `:set color.template=black-background`, `:set color.template=white-background`, etc. And then from there, those "MACRO" command will trigger a series of set error, message, gremlin, etc.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
I think I'll leave templating for another jira ticket. :satisfied: Currently, themes (really any `:set` commands) can be saved into files and loaded with `:load my-gremlin.theme`.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
I'm done with changes. Carry on.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/384
@robertdale did you test this on Windows?
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/384
@pluradj sucks the wind out of the PR - hahaha
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/384
hoping I don't have to fire up that old machine
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/384
Good news, it works on Windows.
Github user pluradj commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/384#discussion_r75548087
— Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy —
@@ -66,12 +72,48 @@ class Console {
public static final String PREFERENCE_ITERATION_MAX = "max-iteration"
private static final int DEFAULT_ITERATION_MAX = 100
private static int maxIteration = DEFAULT_ITERATION_MAX
+
+ public static final String PREF_GREMLIN_COLOR = "gremlin.color"
+ def gremlinColor =
+
— End diff –
Got a mix of tab and space indenting going on here. The [dev docs](http://tinkerpop.apache.org/docs/current/dev/developer/#_code_style) don't call for one over the other, but most Java/Groovy files in the repo use spaces only.
Github user PommeVerte commented on the issue:
https://github.com/apache/tinkerpop/pull/384
awesome!! VOTE: +1 from me as well
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/384
The colorization stuff looks good.
One small problem I've run into is that you can't set a prompt string that contains spaces. For example, I tried `:set result.prompt 'result => '` and got this error message:
```Command ':set' requires arguments: <name> [<value>]```
Setting the preference containing spaces from `groovysh` works fine. It appears to be specific to Gremlin Console, and it is reproducible in 3.2.1 without this PR. Would be fine to handle this bug as a separate JIRA.
Github user robertdale commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/384#discussion_r75560386
— Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/GremlinGroovysh.groovy —
@@ -45,12 +46,13 @@ class GremlinGroovysh extends Groovysh {
Command findCommand(final String line, final List<String> parsedArgs = null) {
def l = line ?: ""
- final List<String> args = parseLine(l)
- if (args.size() == 0) return null
+ final List<String> linetokens = parseLine(l)
+ if (linetokens.size() == 0) return null
- def cmd = registry.find(args[0])
+ def cmd = registry.find(linetokens[0])
- if (cmd != null && args.size() > 1 && parsedArgs != null) {
+ if (cmd != null && linetokens.size() > 1 && parsedArgs != null) {
+ List<String> args = CommandArgumentParser.parseLine(line, parsedArgs == null ? 1 : -1)-
- End diff –
-
groovysh added this line 1 month after this code was written.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
The previous commit to fix :set with quoted spaces did break :remote commands because the parser stripped the quotes. Since :set command is the only one affected, I made a quick hack for it while leaving everything else alone. If this is not acceptable, just let me know and I'll back it out.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/384
someone might want to run:
```
mvn clean install -pl gremlin-console -DskipIntegrationTests=false
```
before merge (now that there are sufficient votes to do so)
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/384
I retested with the latest commits. Setting a preference value with spaces worked, remotes worked nicely with `conf/remote-objects.yaml`. Nice work @robertdale.
`mvn clean install -pl gremlin-console -DskipIntegrationTests=false` was successful.
One other thought here even though I've already voted... it might be better if the preference keys were all prefixed with `gremlin.` since Groovy preferences are [globally shared](https://github.com/apache/groovy/blob/master/src/main/org/codehaus/groovy/tools/shell/util/Preferences.java#L34) with other groovysh-based consoles. That preference problem may have hit us before with [`interpreterMode`](https://github.com/thinkaurelius/titan/issues/1172).
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/384
@pluradj I noticed that too. I was thinking that metaclass could be used to intercept calls to groovy shell's Preferences class and gremlin console could have its own Java Preferences node. This would give it full separation from normal 'groovysh'. I believe that currently max-iteration is the only gremlin-specific pref. So if we do decide to use namespaces, then max-iteration should be deprecated but supported with and without namespace.
I've tried to do this a couple times for fun, but it never seems to want to work. Not sure what we are doing differently than groovysh.
Also not sure what we would want to color exactly but was hoping to at least flip on the capability and then there could be discussion about what we might want to add color to. So I think that's what this ticket could be about if someone feels like trying to tackle it.