Add command-line parser #123

Xrayez · 2021-08-07T14:34:25Z

Port of godotengine/godot#44594 to Godot 3.x to be available in Goost, with another bulk of changes and enhancements.

Co-authored-by: https://github.com/Shatur and https://github.com/lupoDharkael, thanks for the work on this as well, it would be too bad for such a feature to remain in the Limbo.

To-do

Further review, adapt and refine implementation.
- Checkers used Callable, which is not present in Godot 3.x
  decided not to implement for now, see d074e05 (#123).
- Automatically use default help format.
Port/write unit tests in GDScript.
Improve documentation.

Port of godotengine/godot#44594 to Godot 3.x. Co-authored-by: Shatur95 <genaloner@gmail.com> Co-authored-by: lupoDharkael <izhe@hotmail.es>

Not properly supported in Godot 3.x.

Note: this could be ported to simple `PackedStringArray` equality comparison in Godot 4.0. [tests skip]

[tests skip]

Consistent with other parsers in Godot, such as json/xml. [tests skip]

Consistent with `Expression.get_error_text()`, and `CommandLineParser`'s own `get_help_text()`. [tests skip]

`get_values/prefixes()` to `get_value/prefix_list()`, make consistent with other `_list()` methods on Godot. `get_forwarded_args()` to `get_forwarding_args()`, make consistent with `allow_forwarding_args` property.

These are not so trivial to implement in Godot 3.x since there's no Callable, which is Godot 4.x feature, and it might be better to see whether checher functionality is really needed in the first place, since validation is also possible to do via code after parsing, which is user-specific.

…xt()`

[ci skip]

There's not much benefit in separating the two, and in most cases makes API more confusing. Current parsing code validates arguments as well anyways.

Allows to conveniently add new options without instantiating `CommandLineOption` classes manually.

…tion()`

[ci skip]

When declarative approach is not enough.

core/command_line_parser.cpp

Shatur · 2021-08-12T13:16:16Z

core/command_line_parser.cpp

-	return OK;
-}
-
-Error CommandLineParser::validate() {
 	for (int i = 0; i < _options.size(); ++i) {


You will not able to define a help method and required arguments without validate. This is how it done in other parsers.

I haven't removed validation code, but merged it at the end of parse(), also renamed validated signal to parsed for options, so more consolidation. As a user, I still don't quite get the usefulness of this to be honest. Could you elaborate?

The validation doesn't look any much different from validation done in parse() to me.

Yes I understand. But without this separation, if you declare a help command and mark some other command as required, the help command will not work because it will fail on required check.

With this separation you can check for the help / version commands first and then call validate to check if other arguments are valid.

Yes I understand. But without this separation, if you declare a help command and mark some other command as required, the help command will not work because it will fail on required check.

Alright, yeah I understand this now. But is it only useful for help/version options, or are there other use cases where this separation may be required?

Like this:

Then the text will be hardcoded :)

As I said, we have both add_help_option() and add_version_option() in public API. Those could be treated as special options and be recognized accordingly by the parser.

Then options will have a switch (maybe internal) to skip validation as I mentioned before.

As to me this behavior (with automatic validation skip) will be less obvious then a direct call to detect required variables (e.g. validate). But it just me.

I've personally never used Boost.program_options, but I did use Python's argparse. There, if you define an option which is required=True, then somehow it still allows to parse --help and it prints a message, so I assume Python does have an internal mechanism to handle such cases. Godot users are more familiar with Python, so I think we should go with this approach.

Also, according to argparse.required documentation:

Required options are generally considered bad form because users expect options to be optional, and thus they should be avoided when possible.

So having API which allows to explicitly validate frowned upon approach for defining options might not be a good idea.

As I said, parse() already does a lot of validation checks for options/arguments prior to validate() (see various internal _validate_* methods), so it's also not 100% clear which checks are purely user-facing to make a better distinction, those that simply set _error_text or just ERR_FAIL_* (incorrect usage of command-line parser itself).

I'd personally like to keep API simpler if there's a possibility to make it simpler without losing functionality. If users say that validate() is needed, it could be added in future versions without breaking compatibility, I guess.

Then options will have a switch (maybe internal) to skip validation as I mentioned before.

I think this is the way to go.

Hmm... This makes.
On second thought a command option to skip validation if the command is specified not that bad.

I have added ability to mark options as "meta", see recent commits. Options that are meta are designed to get information about application itself, so if any such option is detected on the command-line, there's no need to check for required options, because meta options are not designed to interact with user options.

I haven't exposed it as a property, should probably stay internal.

Awesome!
I would allow user to set it, cost nothing, but it's up to you.

core/command_line_parser.cpp

Xrayez · 2021-08-13T12:44:59Z

I feel like current implementation and API is fine.

So lets merge this and make it go through https://github.com/goostengine/goost-fuzzer to reveal potential crashes in gd3 branch.

Thanks for providing implementation and going through review process, hopefully we have consensus here. 🙂

Shatur · 2021-08-13T12:47:02Z

Thanks for providing implementation and going through review process, hopefully we have consensus here. 🙂

You are welcome :)

Xrayez added feature 💡 New feature proposal topic:core labels Aug 7, 2021

Xrayez added this to the 1.1-gd3 milestone Aug 7, 2021

Xrayez marked this pull request as draft August 7, 2021 14:34

Xrayez mentioned this pull request Aug 8, 2021

Godot's command-line debugger does not quit application when passing quit command immediately godotengine/godot#51387

Open

Xrayez force-pushed the command-line-parser branch from b07f750 to f193f83 Compare August 8, 2021 13:26

Xrayez mentioned this pull request Aug 8, 2021

Add command-line parser class godotengine/godot#44594

Closed

Xrayez force-pushed the command-line-parser branch from 4f6d9ad to 350829c Compare August 9, 2021 12:55

Xrayez and others added 22 commits August 9, 2021 23:59

Add command-line parser

d7d64e2

Port of godotengine/godot#44594 to Godot 3.x. Co-authored-by: Shatur95 <genaloner@gmail.com> Co-authored-by: lupoDharkael <izhe@hotmail.es>

Remove override keywords in CommandLineParser

c2fc422

Not properly supported in Godot 3.x.

Fix shadowed variable when comparing option names

c7ff6ab

Note: this could be ported to simple `PackedStringArray` equality comparison in Godot 4.0. [tests skip]

Formatting and style changes to CommandLineParser

6f8d80a

[tests skip]

Fix typo in get_occurrence_count in CommandLineParser

e10edfd

[tests skip]

Documentation fixes for command-line parser related classes

416d1e2

Rename parse_args to parse in CommandLineParser

807c142

Consistent with other parsers in Godot, such as json/xml. [tests skip]

Add CommandLineParser test suite

4fe0eb3

Rename get_error() to get_error_text() in CommandLineParser

02d8645

Consistent with `Expression.get_error_text()`, and `CommandLineParser`'s own `get_help_text()`. [tests skip]

Add test cases for CommandLineParser

7be1c72

Rename more method in CommandLineParser

df0a2b3

`get_values/prefixes()` to `get_value/prefix_list()`, make consistent with other `_list()` methods on Godot. `get_forwarded_args()` to `get_forwarding_args()`, make consistent with `allow_forwarding_args` property.

Add short option and positional test cases in CommandLineParser

db474d3

Make help format parameter optional in `CommandLineParser.get_help_te…

9c02f16

…xt()`

Move one-line implementation to declaration in CommandLineParser

e0f102a

Update docs for CommandLineParser

66e37de

[ci skip]

Use ERR_PARSE_ERROR for CommandLineParser

7141f6e

Merge validate() into parse() in CommandLineParser

8b01b14

There's not much benefit in separating the two, and in most cases makes API more confusing. Current parsing code validates arguments as well anyways.

Add new_option() to CommandLineParser

dcf414e

Allows to conveniently add new options without instantiating `CommandLineOption` classes manually.

Change getter names to follow naming conventions in command line parser

d6cb740

Use index over idx for CLI option arguments

1a0fa2d

Declare CommandLineParser dependencies

093b4d2

Xrayez force-pushed the command-line-parser branch from 4ecbbf6 to 093b4d2 Compare August 9, 2021 21:06

Xrayez added 9 commits August 10, 2021 21:40

Add more test cases for command line parser

76a7e5c

Rename add_option() to append_option(), new_option() to `add_op…

b66bbef

…tion()`

Fix usage example of command-line parser

05f4b44

Further improve docs for command-line parser

1b77d73

Add prefix test cases for command-line parser

b05da1d

Fix ParsedPrefix.is_exists() name

514d73b

Add important TODOs for Godot 4.0 regarding command-line parser

ff7ee37

[ci skip]

Add utility methods for CommandLineOption

1a41e9d

When declarative approach is not enough.

Validate disallowed default arguments in CommandLineOption

2eeadba

Shatur reviewed Aug 12, 2021

View reviewed changes

core/command_line_parser.cpp Outdated Show resolved Hide resolved

Shatur reviewed Aug 12, 2021

View reviewed changes

core/command_line_parser.cpp Outdated Show resolved Hide resolved

Xrayez added 7 commits August 12, 2021 16:54

Add test case for unrecognized options

8e4595c

Move some variable closer to use in command-line parser

558dd3d

Mark help and version as special meta options

37b7478

Make sure meta option was parsed on the command-line

f539979

Expose meta property in CommandLineOption

a4859a6

Add positional options test case for command-line parser

20d3a30

Simplify implementation of CommandLineParser.add_option() method

90ea98a

Xrayez marked this pull request as ready for review August 13, 2021 09:54

Fix precedence of long prefix parsing in CommandLineParser

9340536

Xrayez merged commit 6ee605b into gd3 Aug 13, 2021

Xrayez deleted the command-line-parser branch August 13, 2021 12:45

Xrayez mentioned this pull request Aug 13, 2021

A list of features approved for Goost development #7

Open

31 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add command-line parser #123

Add command-line parser #123

Xrayez commented Aug 7, 2021 •

edited

Loading

Shatur Aug 12, 2021 •

edited

Loading

Xrayez Aug 12, 2021 •

edited

Loading

Shatur Aug 12, 2021

Shatur Aug 12, 2021 •

edited

Loading

Xrayez Aug 12, 2021

Shatur Aug 12, 2021 •

edited

Loading

Xrayez Aug 12, 2021 •

edited

Loading

Shatur Aug 12, 2021

Xrayez Aug 12, 2021 •

edited

Loading

Shatur Aug 12, 2021

Xrayez commented Aug 13, 2021

Shatur commented Aug 13, 2021

Add command-line parser #123

Add command-line parser #123

Conversation

Xrayez commented Aug 7, 2021 • edited Loading

To-do

Shatur Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Xrayez Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Shatur Aug 12, 2021

Choose a reason for hiding this comment

Shatur Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Xrayez Aug 12, 2021

Choose a reason for hiding this comment

Shatur Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Xrayez Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Shatur Aug 12, 2021

Choose a reason for hiding this comment

Xrayez Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Shatur Aug 12, 2021

Choose a reason for hiding this comment

Xrayez commented Aug 13, 2021

Shatur commented Aug 13, 2021

Xrayez commented Aug 7, 2021 •

edited

Loading

Shatur Aug 12, 2021 •

edited

Loading

Xrayez Aug 12, 2021 •

edited

Loading

Shatur Aug 12, 2021 •

edited

Loading

Shatur Aug 12, 2021 •

edited

Loading

Xrayez Aug 12, 2021 •

edited

Loading

Xrayez Aug 12, 2021 •

edited

Loading