Skip to content
This repository has been archived by the owner on Jan 19, 2024. It is now read-only.

Optional arguments should be in square brackets, required arguments should be in angle brackets. #50

Closed
wants to merge 2 commits into from

Conversation

mikelik
Copy link
Contributor

@mikelik mikelik commented Apr 26, 2023

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:

$ ./antler-proj init --help
Initialize a new project creating the directory tree and a `project.yaml` file.
Usage: antler-proj init [OPTIONS] path project_name [version]

Positionals:
  path <TEXT>                 The root path to create the project in.
  project_name <TEXT>         The name of the project.
  version [TEXT], default: 0.0.0
                              The version to store in the project file.

Options:
  -h,--help                   Print this help message and exit
  -p <TEXT>                   The root path to create the project in.
  -n <TEXT>                   The name of the project.
  -v [TEXT], default: 0.0.0   The version to store in the project file.

Examples:
	antler-proj init MyProjectName 1.0.0

…ckets, required arguments should be in angle brackets.
@mikelik mikelik self-assigned this Apr 26, 2023
std::string make_option_opts(const CLI::Option* opt) const override;
};

std::string BnfFormatter::make_option_opts(const CLI::Option* opt) const {
Copy link
Contributor Author

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

class Formatter : public FormatterBase {

changes are for handling get_required and get_default_str

Copy link
Contributor

@ScottBailey ScottBailey left a 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.

Copy link
Contributor

@ScottBailey ScottBailey left a 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.

Copy link
Contributor

@jolly-fellow jolly-fellow left a 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 []

@mikelik
Copy link
Contributor Author

mikelik commented Apr 27, 2023

I'm fine with cancelling this PR together with issue, as I do not feel it gives benefit.

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.

Will #13 cover it fully or partially? If partially we might want to create a seperate issue.

Not necessarily in this PR: we may want to add type_name_fn() to options.

Created issue: #51

@jolly-fellow
Copy link
Contributor

jolly-fellow commented Apr 27, 2023

I'm fine with cancelling this PR together with issue, as I do not feel it gives benefit.

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.

Will #13 cover it fully or partially? If partially we might want to create a seperate issue.

Not necessarily in this PR: we may want to add type_name_fn() to options.

Created issue: #51

I have updated description of #13 because the parser was significantly changed since I wrote it.
IMHO we should make a meeting where agree how the parser should look in details based on all the registered bugs, write this agreement in a file and then update the parser as close to the agreement as possible but without overengineering.
Or we can review and retest all the issues which takes the parser, then write a description how the parser should looks to cover all these issues, we discuss this description and then update the parser as agreed.
Or at least collect in one text all requirements, wishes and problems of the CLI to have a base for discussion.
It looks that fixing of the bugs one by one doesn't allow to find a general solution and we will forced to rewrite the same several times.
Well IMHO it would be better to talk about this problem together and then decide what to do.

@mikelik
Copy link
Contributor Author

mikelik commented Apr 27, 2023

Cancelling this PR, as the description of original issue has changed.
I will raise a new PR to fix the description of remove

@mikelik mikelik closed this Apr 27, 2023
@mikelik mikelik deleted the mikelik/help_fix branch April 27, 2023 10:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Fix description of antler-proj remove
3 participants