Details

    • Improvement
    • Status: Closed
    • Trivial
    • Resolution: Done
    • 3.1.0-incubating
    • 3.2.2
    • console
    • None

    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

          Activity

            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.

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

            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


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

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

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

            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

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

            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)

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

            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)?

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

            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.

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

            Github user alaam commented on the issue:

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

            +1 for having this be a preferences file

            githubbot ASF GitHub Bot added a comment - Github user alaam commented on the issue: https://github.com/apache/tinkerpop/pull/384 +1 for having this be a preferences file
            githubbot ASF GitHub Bot added a comment -

            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.

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

            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) ?

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

            Github user spmallette commented on the issue:

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

            +1 for use of `:set`

            githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/384 +1 for use of `:set`
            githubbot ASF GitHub Bot added a comment -

            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.

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

            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)

              1. Color codes:
            • BLACK
            • RED
            • GREEN
            • YELLOW
            • BLUE
            • MAGENTA
            • CYAN
            • WHITE
                1. Background Colors
            • BG_BLACK
            • BG_RED
            • BG_GREEN
            • BG_YELLOW
            • BG_BLUE
            • BG_MAGENTA
            • BG_CYAN
            • BG_WHITE
                1. 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
            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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:

            https://github.com/apache/tinkerpop/blob/f35f733663029701af538fc527e1f5ab563eb07d/docs/src/reference/gremlin-applications.asciidoc#console-commands

            then maybe an announcement of the feature in upgrade docs for users:

            https://github.com/apache/tinkerpop/blob/f35f733663029701af538fc527e1f5ab563eb07d/docs/src/upgrade/release-3.2.x-incubating.asciidoc#upgrading-for-users

            and if you want to really be complete an entry in changelog:

            https://github.com/apache/tinkerpop/blob/f35f733663029701af538fc527e1f5ab563eb07d/CHANGELOG.asciidoc#tinkerpop-322-not-officially-released-yet

            @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:

            githubbot ASF GitHub Bot added a comment - 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: https://github.com/apache/tinkerpop/blob/f35f733663029701af538fc527e1f5ab563eb07d/docs/src/reference/gremlin-applications.asciidoc#console-commands then maybe an announcement of the feature in upgrade docs for users: https://github.com/apache/tinkerpop/blob/f35f733663029701af538fc527e1f5ab563eb07d/docs/src/upgrade/release-3.2.x-incubating.asciidoc#upgrading-for-users and if you want to really be complete an entry in changelog: https://github.com/apache/tinkerpop/blob/f35f733663029701af538fc527e1f5ab563eb07d/CHANGELOG.asciidoc#tinkerpop-322-not-officially-released-yet @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:
            githubbot ASF GitHub Bot added a comment -

            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)

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

            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?

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

            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.

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

            Github user dkuppitz commented on the issue:

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

            Oh nice, I like that.

            VOTE: +1

            githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/384 Oh nice, I like that. VOTE: +1
            githubbot ASF GitHub Bot added a comment -

            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)

            githubbot ASF GitHub Bot added a comment - 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 )
            jeromatron Jeremy Hanna added a comment - - edited

            Great to see all of this come together. Thanks rdale spmallette dkuppitz okram!

            jeromatron Jeremy Hanna added a comment - - edited Great to see all of this come together. Thanks rdale spmallette dkuppitz okram !
            githubbot ASF GitHub Bot added a comment -

            Github user alaam commented on the issue:

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

            +1

            githubbot ASF GitHub Bot added a comment - Github user alaam commented on the issue: https://github.com/apache/tinkerpop/pull/384 +1
            githubbot ASF GitHub Bot added a comment -

            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.

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

            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.

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

            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.

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

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

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

            Github user robertdale commented on the issue:

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

            I'm done with changes. Carry on.

            githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/384 I'm done with changes. Carry on.
            githubbot ASF GitHub Bot added a comment -

            Github user okram commented on the issue:

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

            VOTE +1.

            githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/384 VOTE +1.
            githubbot ASF GitHub Bot added a comment -

            Github user pluradj commented on the issue:

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

            @robertdale did you test this on Windows?

            githubbot ASF GitHub Bot added a comment - Github user pluradj commented on the issue: https://github.com/apache/tinkerpop/pull/384 @robertdale did you test this on Windows?
            githubbot ASF GitHub Bot added a comment -

            Github user spmallette commented on the issue:

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

            @pluradj sucks the wind out of the PR - hahaha

            githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/384 @pluradj sucks the wind out of the PR - hahaha
            githubbot ASF GitHub Bot added a comment -

            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

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

            Github user pluradj commented on the issue:

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

            Good news, it works on Windows.

            githubbot ASF GitHub Bot added a comment - Github user pluradj commented on the issue: https://github.com/apache/tinkerpop/pull/384 Good news, it works on Windows.
            githubbot ASF GitHub Bot added a comment -

            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 =

            { Preferences.get(PREF_GREMLIN_COLOR, "reset") }

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

            githubbot ASF GitHub Bot added a comment - 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 = { Preferences.get(PREF_GREMLIN_COLOR, "reset") } + — 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.
            githubbot ASF GitHub Bot added a comment -

            Github user PommeVerte commented on the issue:

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

            awesome!! VOTE: +1 from me as well

            githubbot ASF GitHub Bot added a comment - Github user PommeVerte commented on the issue: https://github.com/apache/tinkerpop/pull/384 awesome!! VOTE: +1 from me as well
            githubbot ASF GitHub Bot added a comment -

            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.

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

            Github user pluradj commented on the issue:

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

            VOTE: +1

            githubbot ASF GitHub Bot added a comment - Github user pluradj commented on the issue: https://github.com/apache/tinkerpop/pull/384 VOTE: +1
            githubbot ASF GitHub Bot added a comment -

            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.

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

            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.

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

            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)

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

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

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

            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.

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

            Github user asfgit closed the pull request at:

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

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

            People

              spmallette Stephen Mallette
              jeromatron Jeremy Hanna
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: