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

Allow command line flags to be used in any order #1

Open
sdushantha opened this issue Sep 13, 2020 · 13 comments
Open

Allow command line flags to be used in any order #1

sdushantha opened this issue Sep 13, 2020 · 13 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@sdushantha
Copy link
Owner

At the moment the flags have to be in a certain order.
For example if I want to view the most recent email and want the output to be as raw text, I have to run tmpmail --text --recent for it work properly. Running tmpmail --recent --text will ignore the --text flag.

By allowing the flags to be used in any order would make it much easier to use.

@sdushantha sdushantha added enhancement New feature or request help wanted Extra attention is needed labels Sep 13, 2020
@BachoSeven
Copy link

BachoSeven commented Sep 13, 2020

You could use getopts(?)

This and also this might be useful

I'll try to implement it and do a PR in case

P.S. My understanding is that the desired flag order is the current order in the case statement

@sdushantha
Copy link
Owner Author

You could use getopts(?)

From my understanding getopts does not support --long or -short flags but only single letter flags: -f

P.S. My understanding is that the desired flag order is the current order in the case statement

I initially thought that was the issue, but after changing the order of the flags in the case statement made no change.

@BachoSeven
Copy link

From my experience, getopts allows -short options in the following sense:
an option defined as -a can be called with -argument and it will work the same way. For an example, see this from a script of mine

@sdushantha
Copy link
Owner Author

Ah yes, you are correct. I think getopt (no 's') supports --long options

@rpdelaney
Copy link
Contributor

rpdelaney commented Sep 13, 2020

This can also be accomplished by looping over the arguments and shifting as we go. Example:

while true ; do
  case ${1:-} in
    --help | -\?)
      usage
      exit 0
      ;;
    -v | --verbose)
      # Each instance of -v adds 1 to verbosity
      verbose=$((verbose+1))
      shift
      ;;
    -q | --quiet)
      quiet="1"
      shift
      ;;
    --) # End of all options
      shift
      break
      ;;
    -*)
      echo "FATAL: Unknown option : $1" >&2
      exit 1
      ;;
    *)  # no more options. Stop while loop
      break
      ;;
  esac
done

@sdushantha
Copy link
Owner Author

I think I've come to a conclusion as to why this this happening and here is the best explanation that I'm able to come up with:

I written a very minimal code below to show what is happening when we bash script.sh --recent --text

RAW_TEXT=false
view_recent_email(){
    echo "The value of RAW_TEXT is --> $RAW_TEXT"       # [Step 2]   
}

while [[ "$1" ]]; do
    case "$1" in
        --recent|-r) view_recent_email ;;               # [Step 1]
        --text|-t) RAW_TEXT=true ;;                     # [Step 3]
    esac
    shift
done

Output:

The value of RAW_TEXT is --> false

Why is the value for RAW_TEXT false?

Because our first argument was --recent. If the --recent flag is given then view_recent_email gets executed. When the function view_recent_email shows the value of RAW_TEXT, it will be false because the value of RAW_TEXT has not been changed to true yet.

The value of RAW_TEXT only becomes true after everything that needs to be done for the --recent flag has been completed.

But, when we run bash script.sh --text --recent, the steps get swapped:

RAW_TEXT=false
view_recent_email(){
    echo "The value of RAW_TEXT is --> $RAW_TEXT"       # [Step 3]   
}

while [[ "$1" ]]; do
    case "$1" in
        --recent|-r) view_recent_email ;;               # [Step 2]
        --text|-t) RAW_TEXT=true ;;                     # [Step 1]
    esac
    shift
done

Output:

The value of RAW_TEXT is --> true

So RAW_TEXT becomes true and then the function view_recent_email gets executed.


Conclusion: We might need to change the code a little so it looks something like this:

RAW_TEXT=false
VIEW_RECENT_EMAIL=false

view_recent_email(){
    echo "The value of RAW_TEXT is --> $RAW_TEXT"      
}

while [[ "$1" ]]; do
    case "$1" in
        --recent|-r) VIEW_RECENT_EMAIL=true ;;               
        --text|-t) RAW_TEXT=true ;;                     
    esac
    shift
done

if [ $VIEW_RECENT_EMAIL = true ];then
	view_recent_email
fi

Outputs:

$ bash script.sh --recent --text
The value of RAW_TEXT is --> true

$ bash script.sh --text --recent
The value of RAW_TEXT is --> true

This allows us to use any order of the flags because the case statement is only being used to set the variables and not to run any functions.


Hope this explanation was understandable

@rpdelaney
Copy link
Contributor

rpdelaney commented Sep 21, 2020

Edit: I misread this, I will try again after my coffee

@rpdelaney
Copy link
Contributor

This allows us to use any order of the flags because the case statement is only being used to set the variables and not to run any functions.

Yes, this is a good approach.

If you ever have parameters that themselves take an argument then you will want to do the shifting inside each case itself, so that you can consume both arguments only when there's an argument that requires that. But in your example, they don't, so shifting at the end of the loop is fine.

@GReagle
Copy link

GReagle commented Oct 2, 2020

I have a branch in my fork in which I changed the command syntax and re-wrote the option parsing. Before I request a pull, I think I'll propose my changes here. The new command syntax, which is documented in a man page (tmpmail.1), is thus:

SYNOPSIS
     tmpmail -a _address_ | -h | -l | -r | -V
     tmpmail [-b _browser_ | -t] -m | -s _ID_
OPTIONS
     -a | --address _address_
             register specified address
     -b | --browser _browser_
             specify browser (default is w3m)
     -h | --help
             help
     -l | --list
             list messages
     -m | --most-recent
             show most recent message
     -r | --random
             register random address
     -s | --show _ID_
             show message with ID
     -t | --text
             text output (default is html)
     -V | --version
             version

Note that the options are presented in alphabetical order. I have reserved lower case -v for verbose mode and used upper case -V for version, as is common. I think that the options parsing code is more clear and detects problems better:

    while [ $# -gt 0 ]; do
        case "$1" in
            -a | --address)
                [ -z "$2" ] && print_error "missing arg for $1" ||
                        generate_email_address true "$2"; exit ;;
            -b | --browser)
                [ -z "$2" ] && print_error "missing arg for $1" ||
                        BROWSER="$2"; shift ;;
            -h | --help)
                usage; exit ;;
            -l | --list)
                list_emails; exit ;;
            -m | --most-recent)
                view_recent_email; exit ;;
            -r | --random)
                generate_email_address true; exit ;;
            -s | --show)
                [ -z "$2" ] && print_error "missing arg for $1" ||
                        view_email "$2"; shift ;;
            -t | --text)
                RAW_TEXT=true ;;
            -V | --version)
                echo "$VERSION"; exit ;;
            -*) print_error "unrecognized option '$1'" ;;
            *)  print_error "unexpected argument '$1'" ;;
        esac
        shift
    done
    usage

What do you think?

@GReagle
Copy link

GReagle commented Oct 2, 2020

I think that this style is easier to read. What do you think?

            -a | --address)
                if [ -z "$2" ]; then print_error "missing arg for $1"
                else generate_email_address true "$2"; exit
                fi;;

@rpdelaney
Copy link
Contributor

            -b | --browser)
                [ -z "$2" ] && print_error "missing arg for $1" ||
                        BROWSER="$2"; shift ;;

I think you will need shift 2 there or else only the switch will be shifted out of the arguments, leaving the user's supplied argument in the array.

@GReagle
Copy link

GReagle commented Oct 3, 2020

I think you will need shift 2 there or else only the switch will be shifted out of the arguments, leaving the user's supplied argument in the array.

Incorrect. You're forgetting the shift that is the last statement of the while loop.

@rpdelaney
Copy link
Contributor

True. I'm not used to doing it that way, but it should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants