Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TINKERPOP-1268 Interactive and Executor Mode for Console #297

Merged
merged 9 commits into from May 2, 2016

Conversation

spmallette
Copy link
Contributor

This PR covers:

https://issues.apache.org/jira/browse/TINKERPOP-1268
https://issues.apache.org/jira/browse/TINKERPOP-1157
https://issues.apache.org/jira/browse/TINKERPOP-1155
https://issues.apache.org/jira/browse/TINKERPOP-1156

I think that the console is working with a nice level of consistency now. We no longer have to warn people that the script they give as an init script is different than a they give to -e - it all executes in the same environment so no more discrepancies.

You can see some examples in the documentation for how -e and -i are used. The shell script simplified a bit - @dkuppitz if you see a better way to deal with the getopts thing please let me know. I need to track -l in the gremlin.sh because we use it to dynamically set some log4j stuff and other debug natured things. getopts kinda feels like overkill but - it's working so.....

I tested windows and it seemed to work.

Please give it a shot and see if you can break it. I tried to test as many combinations as I could think of but i may have fell short somewhere.

VOTE + 1

Added -i for interactive mode and made -e execute in the Console rather than ScriptExecutor. Added switches to show/hide output and added a switch for "help". The existing method for -i still works as in "bin/gremlin.sh init.groovy"
Only override if the verbosity is not explicitly set.
@dkuppitz
Copy link
Contributor

Looks good to me. I prefer this approach for option parsing:

https://github.com/apache/incubator-tinkerpop/blob/master/docker/scripts/build.sh#L40-L50

However, it requires that you handle all possible options and show a usage message if an option is unknown. That probably doesn't fit well in gremlin.sh, as the Java side is supposed to handle all "unknown" options and you likely don't want to keep the shell script and the Java side in sync whenever an option is added / removed.

VOTE: +1

@spmallette
Copy link
Contributor Author

yeah - i thought about adding all the options to getopts and i did not like that idea especially since the console also supports long variants of each of the items:

bin/gremlin.sh -e script.groovy
bin/gremlin.sh --execute=script.groovy

was getting too complicated to try to maintain that.

@spmallette
Copy link
Contributor Author

I mean to write this earlier....note that the addition of:

        <dependency>
            <groupId>commons-cli</groupId>
            <artifactId>commons-cli</artifactId>
            <version>1.2</version>
        </dependency>

does affect the binary distribution for the console but it does not affect LICENSE/NOTICE. The NOTICE file included in commons-cli is boilerplate apache and therefore does not need to be included in our NOTICE.

if (scriptAndArgs.size() > 1) {
List<String> args = scriptAndArgs.subList(1, scriptAndArgs.size())
groovy.execute("args = [\"" + args.join('\",\"') + "\"]")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should declare args as either null or [] here if none are passed in, otherwise a script allowing 0 or more args would hit an error for args not declared.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's done in L166.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm suggesting this

            if (scriptAndArgs.size() > 1) {
                List<String> args = scriptAndArgs.subList(1, scriptAndArgs.size())
                groovy.execute("args = [\"" + args.join('\",\"') + "\"]")
            } else {
                groovy.execute("args = []")
            }

so that this input groovy script

println args

doesn't fail like this

/bin/gremlin.sh -e /tmp/foo.groovy
Error in /tmp/foo.groovy at [1: println args] - No such property: args for class: groovysh_evaluate
groovy.lang.MissingPropertyException: No such property: args for class: groovysh_evaluate

@pluradj
Copy link
Member

pluradj commented May 1, 2016

Looks good, Stephen. I reviewed the code, reviewed the docs, tested on Mac and Windows.

VOTE: +1

@asfgit asfgit merged commit 1e83a32 into master May 2, 2016
@asfgit asfgit deleted the TINKERPOP-1268 branch August 3, 2016 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants