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

Relaxed option naming #627

Merged
merged 8 commits into from
Aug 23, 2021
Merged

Relaxed option naming #627

merged 8 commits into from
Aug 23, 2021

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Jul 21, 2021

If merged this pull request will relax the restrictions on option naming and subcommand naming.

Allowing inclusion of all characters except
starting with:

  • - since we need to recognize - and -- as different options and -- has some special meaning
  • \n since that would cause all sorts of chaos with the formatters and other things
  • ! used as a special character for setting default values
  • allowing this would create the potential for difficult to debug user entry errors for example "-- input, input" would be adding an extra space accidentally, But if space were allowed this would read fine as a single almost impossible to use option.

and anywhere in the option

  • \n same as above
  • =, and : they are fine as a first character but after that they are used as a value separator so can't be allowed in an option
  • { {} is used to indicate default values
  • allowing this would create the potential for difficult to debug user entry errors for example "--input input" would be forgetting a comma to separate an option from the positional. But if space were allowed this would read fine as a single almost impossible to use option.

this will address #580
also allow lots of amusing things like --:) as an option

A couple questions in #592 we talked about adding a check on group_names and a few other things. particularly in regard to newlines. I need to do that yet.

But in regard to subcommands through the add_subcommand I am inclined to leave the check for spaces in the subcommand name in place. The reason has to do with error catching. Most subcommand names don't have spaces but their description does quite frequently. Meaning the check on spaces catches some potential errors in the API call. Removing this restriction would likely cause additional errors that might be hard for a user to spot initially.

Then the question is should we allow newlines in App names? If not might be worth adding an additional validator to catch that and use it for app names, aliases, and group names.

@phlptp
Copy link
Collaborator Author

phlptp commented Jul 21, 2021

TODO

  • readme updates
  • additional test cases for subcommands
  • validators for group names, app name, aliases
  • book updates

@phlptp phlptp force-pushed the relaxedOptionNaming branch 3 times, most recently from eb47a61 to 32cdab3 Compare July 30, 2021 23:05
@phlptp phlptp marked this pull request as ready for review July 31, 2021 13:23
@phlptp
Copy link
Collaborator Author

phlptp commented Jul 31, 2021

Added a validator for group_names, aliases, and option group names. But not app names. App names can still have new lines, so it is possible to get a subcommand with a new line but not easy. Should we restrict this as well?

book/chapters/flags.md Show resolved Hide resolved
book/chapters/options.md Outdated Show resolved Hide resolved
include/CLI/App.hpp Outdated Show resolved Hide resolved
include/CLI/Option.hpp Outdated Show resolved Hide resolved
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
@henryiii henryiii merged commit 19047d8 into CLIUtils:master Aug 23, 2021
@github-actions github-actions bot added the needs changelog Hasn't been added to the changelog yet label Aug 23, 2021
@henryiii henryiii removed the needs changelog Hasn't been added to the changelog yet label Sep 29, 2021
@phlptp phlptp deleted the relaxedOptionNaming branch March 11, 2023 01:26
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