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

Adding the run-jshell script to the bin/ #85

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

jwgibbs
Copy link
Contributor

@jwgibbs jwgibbs commented Aug 21, 2023

Added a bash script called run-shell that will open the jshell interactive session and load the jar files in coatjava/lib to the class-path. The user does not need to add the jar files as options on the command line or set them as environment variables.

bin/run-jshell Outdated
echo "*****************************************"
echo " "
echo " "
jshell --class-path "$JYPATH" $*
Copy link
Member

Choose a reason for hiding this comment

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

Using "$@" should allow better argument capture (see #79).

Suggested change
jshell --class-path "$JYPATH" $*
jshell --class-path "$JYPATH" "$@"

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, resolved (not sure why I don't see the "mark as resolved button", but if you have it, please click it, otherwise one of the repo admins will need to).

@jwgibbs
Copy link
Contributor Author

jwgibbs commented Aug 21, 2023

Made the recommended edit. Switched "$*" with "$@" for the command line inputs.

@jwgibbs jwgibbs closed this Aug 21, 2023
@jwgibbs
Copy link
Contributor Author

jwgibbs commented Aug 21, 2023 via email

@c-dilks
Copy link
Member

c-dilks commented Aug 21, 2023

I clicked the Closed with Comment, and now the pull request is marked as Closed. I don’t know if that did anything.

"Closed" means this Pull Request will no longer be considered for merging; you should re-open it (unless you no longer want it); I believe there is a button near the bottom to do this.

@c-dilks
Copy link
Member

c-dilks commented Aug 21, 2023

On the other hand, my comments above were part of a "Review", and "Review Conversations" typically need to be "Resolved" before the maintainers can merge a Pull Request. You've added the commit, but there should be a "Resolve" button somewhere near my comments above (if not, the maintainers can do this).

@jwgibbs jwgibbs reopened this Aug 21, 2023
echo "*****************************************"
echo " "
echo " "
jshell --class-path "$JYPATH" "$@"
Copy link
Member

@c-dilks c-dilks Aug 21, 2023

Choose a reason for hiding this comment

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

Do you need to export JAVA_OPTS too, as bin/run-groovy does?

On that note, this script has significant overlap with bin/run-groovy, which makes me wonder if they could be combined, e.g., move common things like JYPATH into bin/env.sh... any thoughts @baltzell?

Copy link
Member

Choose a reason for hiding this comment

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

From some discussion today, we are fine with the repetition.

@jwgibbs
Copy link
Contributor Author

jwgibbs commented Aug 21, 2023 via email

@baltzell baltzell merged commit f650a0c into JeffersonLab:development Aug 28, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants