-
Notifications
You must be signed in to change notification settings - Fork 0
Optional arguments should be in square brackets, required arguments should be in angle brackets. #50
Conversation
…ckets, required arguments should be in angle brackets.
std::string make_option_opts(const CLI::Option* opt) const override; | ||
}; | ||
|
||
std::string BnfFormatter::make_option_opts(const CLI::Option* opt) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is mainly taken from
Line 4093 in 286ab12
class Formatter : public FormatterBase { |
changes are for handling
get_required
and get_default_str
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure this is correct:
beauty$ aproj remove --help
Remove an app, dependency, library or test from your project.
Usage: antler-proj remove [OPTIONS] [path] [SUBCOMMAND]
Positionals:
path [TEXT] Path containing the project's yaml file.
Options:
-h,--help Print this help message and exit
-p [TEXT] Path containing the project's yaml file.
Subcommands:
app Remove app from the project.
lib Remove lib from the project.
dep Remove a dependency from the project.
I think the original behavior for -p
was acceptable:
Options:
-h,--help Print this help message and exit
-p TEXT Path containing the project's yaml file.
Or this is fine, too:
Options:
-h,--help Print this help message and exit
-p <TEXT> Path containing the project's yaml file.
I say this because TEXT is no longer optional when we add the -p
flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily in this PR: we may want to add type_name_fn()
to options.
subcommand->add_option("-p, path", path, "Path containing the project's yaml file.")
->type_name_fn([]()->std::string{ return "PATH"; });
to report as so:
Options:
-h,--help Print this help message and exit
-p [PATH] Path containing the project's yaml file.
Instead of TEXT
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh. This solution looks a little bit as an overkill IMHO. When I wrote #14 I didn't know abilities of the CLI11 so it was my wish to follow BNF notation if possible. But if the CLI library doesn't support BNF IMHO it's OK to use it "as is" instead of adding our own support. Anyway there are many popular CLI programs which don't support it too.
IMHO the bigger problem is duplication of the options. IMHO we should leave only one way to declare arguments: options or positionals, because both of them make the interface confusing and difficult to understand. I prefer options instead of positionals because they are explicit and make less problems with parsing. It is easily to recognize an option and its parameter than correctly parse positional argument and don't confuse them.
And anyway the new help text is incorrect IMHO.
The positionals should look like:
Positionals:
<path> The root path to create the project in.
<project_name> The name of the project.
[version] The version to store in the project file. Default: 0.0.0
Because they are not options which have names but positional arguments which recognizing by their position in the command line.
Options:
-v <version>, default: 0.0.0 The version to store in the project file.
Because parameter must be declared with -v option. -v can't be used without a parameter so <> instead of []
I'm fine with cancelling this PR together with issue, as I do not feel it gives benefit.
Will #13 cover it fully or partially? If partially we might want to create a seperate issue.
Created issue: #51 |
…ch will be cancelled.
I have updated description of #13 because the parser was significantly changed since I wrote it. |
Cancelling this PR, as the description of original issue has changed. |
Resolves #14
Modify formatter: optional arguments should be in square brackets, required arguments should be in angle brackets.
Fixed help for
remove
.Example help for
init
: