ctubbsii commented on a change in pull request #1910:
URL: https://github.com/apache/accumulo/pull/1910#discussion_r583681393
##########
File path: assemble/bin/accumulo
##########
@@ -58,6 +58,18 @@ function main() {
echo "$CLASSPATH"
exit 0
fi
+
+ # Set up path variable for default import config file
+ export jShellPath="$conf/jshell-init.jsh"
+
+ if [[ $cmd == "jshell" ]]; then
+ shift
+ if [[ -f "$jShellPath" ]]; then
+ exec "$cmd" --startup DEFAULT "$jShellPath" "$@"
Review comment:
I think `--startup DEFAULT` is a redundant option, since it is the
default behavior.
I was thinking `--startup "$jShellPath"` could be used instead of `--startup
DEFAULT`, as its replacement. So, the choice we have is to use the default
startup or to replace it.
This really comes down to [the difference between an 'option' and a
'parameter'](https://stackoverflow.com/a/36495940/196405), and the fact that
the man page says `jshell [options] [load-files]`, implying options should
precede file parameters, as is typical for command-line programs.
To explain why this matters, consider the following. In my explanation, I
replace the `--startup DEFAULT` option with the `--no-startup` option, to show
the differences between options and file parameters, while ignoring the
behavioral differences between including the `DEFAULT` startup or not.
Functionally, the following are the same:
```
jshell --startup conf/jshell-init.jsh <-- replaces the DEFAULT startup
with this file
jshell --no-startup conf/jshell-init.jsh <-- skips the DEFAULT startup and
only executes this file
```
However, if we're also passing in user options in `$@`, this becomes a
problem. To see why, imagine the user executes `bin/accumulo jshell -v
userFile.jsh`, then the resulting commands look like:
```
jshell --startup conf/jshell-init.jsh -v userFile.jsh <-- replaces the
DEFAULT startup, also sets the verbose feedback mode, and then executes the
user file
jshell --no-startup conf/jshell-init.jsh -v userFile.jsh <-- may work, but
mixes [options] with [load-files]
```
I actually tried these both, and jshell did work for me, even though it is
not correct according to the manual. It seems jshell is a bit forgiving, at
least in the version I have installed. However, we shouldn't count on it
working.
And, there is still a difference. If you run `/list -all`, you can see that
anything executed as a startup script is listed with the letter `s` followed by
a number, and anything executed after, including all commands from
`[load-files]` parameters, just gets a number. This matters for user
experience, because if you want to reset the state of the jshell using
`/reset`, all the startup scripts will re-execute, but any `[load-files]` ones
will not (`/reload` would be used instead of `/reset` if you want to re-run the
other commands after resetting).
So, this is all to say that I think that we should make sure that when we
execute `jshell-init.jsh`, we should ensure that it is done as a `--startup`
option, and not as a `[load-files]` parameter. That way, `/reset` and `/reload`
behave as expected, and we don't mix and match `[options]` and `[load-files]`
arguments when users specify additional options to the command, which may not
work in future.
The only downside to this is that if we want any `java.*` imports
automatically loaded, we need to include them in the generated file. However,
this isn't really a downside, as much as it is an opportunity to tailor our
initial imports to avoid any potential conflicts (such as that between
`java.util.Scanner` and `org.apache.accumulo.core.client.Scanner`).
```suggestion
exec "$cmd" --startup "$jShellPath" "$@"
```
##########
File path: assemble/bin/accumulo
##########
@@ -58,6 +58,18 @@ function main() {
echo "$CLASSPATH"
exit 0
fi
+
+ # Set up path variable for default import config file
+ export jShellPath="$conf/jshell-init.jsh"
+
+ if [[ $cmd == "jshell" ]]; then
+ shift
+ if [[ -f "$jShellPath" ]]; then
+ exec "$cmd" --startup DEFAULT "$jShellPath" "$@"
Review comment:
I just learned that we can actually specify more than one `--startup`
option, so if we don't want to customize the `java.*` imports for now, and just
want the defaults, we can do:
```suggestion
exec "$cmd" --startup DEFAULT --startup "$jShellPath" "$@"
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
