Skip to content
This repository has been archived by the owner on Apr 6, 2022. It is now read-only.

Convention of optional args #26

Open
napulen opened this issue May 19, 2021 · 8 comments
Open

Convention of optional args #26

napulen opened this issue May 19, 2021 · 8 comments
Assignees
Labels
low priority question Further information is requested

Comments

@napulen
Copy link
Member

napulen commented May 19, 2021

The general convention for optional args in a (unix-like) CLI is:

  • -o single-letter switch for single-dashed options and
  • --option-in-cli dashed-separated words for double-dashed options

These usually go in pairs, the shorthand and the full spelled option name. For example:

-h | --help
-v | --version

The current notation for -Wtxt or -export conflicts with this convention.

For -Wtxt I would recommend to have a separate switch specifying the type of file. For example

-W -t txt; where -t means type and it can be a choice between ["mei", "txt"] or something similar.

@napulen
Copy link
Member Author

napulen commented May 19, 2021

For the -export, --export would work.

@napulen
Copy link
Member Author

napulen commented May 19, 2021

If you have arguments for choosing the current convention, explain them and if they're compelling, the current convention can stay.

@napulen napulen added this to the MEI2Volpiano as a Rodan Job milestone May 19, 2021
@napulen napulen added question Further information is requested low priority labels May 19, 2021
@raviraina
Copy link
Contributor

raviraina commented May 19, 2021

@napulen Should be resolved in the most recent commits to dev, let me know if this resolves the issue fully!

@napulen
Copy link
Member Author

napulen commented May 19, 2021

At a first glance

    parser.add_argument(
        "-t",
        nargs="?",
        required=True,
        type=lambda fname: check_file_validity(fname, ["txt", "mei"]),
        help="Flag indicating whether the inputs will be mei or txt files",
    )

that could be

    parser.add_argument(
        "-t",
        nargs="?",
        required=True,
        choices=["txt", "mei"],
        help="Flag indicating whether the inputs will be mei or txt files",
    )

Here is the relevant doc: https://docs.python.org/3/library/argparse.html#choices

Not sure about the required=True. By definition, an option specified by a dash is an optional argument. Need to look at your logic further to assess both use cases (the lambda and the required).

@raviraina
Copy link
Contributor

raviraina commented May 19, 2021

Yeah I left the -t and set it as required just to have it stick with a flag as you had done in the initial issue comment, I can change it up though. In regards to the choices option, I will change it to that.. got caught up trying to reuse what I had built previously.

EDIT: In fact, upon second glance I can remove the whole use of the check_file_validity() function and simplify the driver further. Changes will be reflected in the next dev commit.

@napulen
Copy link
Member Author

napulen commented May 19, 2021

No problem. Looking better already. In the README too 👍

Re. the type, if you want to enforce a certain type by default without making it a required arg, you can just set default="mei" or similar. Analog to setting a default value on an input argument to a function def foo(requiredArg, optional="mei").

@raviraina
Copy link
Contributor

raviraina commented May 20, 2021

@napulen updated so that instead of providing a -t, the field is set default to mei, (so mei2vol -W filename.mei or mei2vol -N filename.mei) and if they wish to input multiple mei through a txt file it would be mei2vol -W txt filename.txt.

EDIT: switched back to specifying mei/txt due to positional errors when passing multiple files in one go.. may come back to it later but the current changes are reflected in the README.

Also was able to remove check_file_validity as it was clearly no longer needed with in current iteration of the driver.

@napulen
Copy link
Member Author

napulen commented May 20, 2021

This one can create ambiguity. The way I read it, the code wouldn't be able to tell if mei2vol txt txt2 is two mei files named txt txt or one mei file of txt type.

In general, the -t with default="mei" seems less obscure to me.

Not a big deal, but pointing it out

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
low priority question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants