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

Use getopts for example scripts #664

Closed
wants to merge 1 commit into from

Conversation

siraben
Copy link
Contributor

@siraben siraben commented Mar 31, 2023

This PR refactors two Bash scripts to use proper argument parsing and fixes some bugs.

The first script (reason-act.sh) used an if-statement to check for the presence of a command-line argument and set a variable accordingly. This has been replaced with getopts to parse the -m option and its argument. The script now uses quotes to prevent word splitting and globbing, and has a shebang at the beginning.

The second script (chat-13B.sh) used several environment variables to specify various settings, and some of them had default values. This has been changed to use getopts to parse command-line options and their arguments. The default values have been moved to the variable definitions. Quotes have been added to variables to prevent word splitting and globbing. An error message has been added for invalid options.

@siraben
Copy link
Contributor Author

siraben commented Apr 1, 2023

cc @prusnak


while getopts "m:u:a:t:p:g:" opt; do
case $opt in
m) MODEL="--model $OPTARG";;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for not doing it like this for consistency:

Suggested change
m) MODEL="--model $OPTARG";;
m) MODEL="$OPTARG";;


./main $MODEL --color \
./bin/main $MODEL --color \
Copy link
Member

Choose a reason for hiding this comment

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

The example scripts are meant to be run from the source root, so this has to remain ./main

@ggerganov
Copy link
Member

Feel free to reopen if the suggestions are resolved, closing to remove clutter

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.

2 participants