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.

Sign up for GitHub

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

Add command-line parser #123

Merged
merged 39 commits into from
Aug 13, 2021
Merged

Add command-line parser #123

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
d7d64e2
Add command-line parser
Xrayez Aug 7, 2021
c2fc422
Remove override keywords in `CommandLineParser`
Xrayez Aug 7, 2021
c7ff6ab
Fix shadowed variable when comparing option names
Xrayez Aug 7, 2021
6f8d80a
Formatting and style changes to `CommandLineParser`
Xrayez Aug 7, 2021
e10edfd
Fix typo in `get_occurrence_count` in `CommandLineParser`
Xrayez Aug 7, 2021
416d1e2
Documentation fixes for command-line parser related classes
Xrayez Aug 7, 2021
807c142
Rename `parse_args` to `parse` in `CommandLineParser`
Xrayez Aug 8, 2021
4fe0eb3
Add `CommandLineParser` test suite
Xrayez Aug 8, 2021
02d8645
Rename `get_error()` to `get_error_text()` in `CommandLineParser`
Xrayez Aug 8, 2021
7be1c72
Add test cases for `CommandLineParser`
Xrayez Aug 8, 2021
df0a2b3
Rename more method in `CommandLineParser`
Xrayez Aug 9, 2021
db474d3
Add short option and positional test cases in `CommandLineParser`
Xrayez Aug 9, 2021
d074e05
Remove dead code related to checkers in `CommandLineParser`
Xrayez Aug 9, 2021
9c02f16
Make help format parameter optional in `CommandLineParser.get_help_te…
Xrayez Aug 9, 2021
e0f102a
Move one-line implementation to declaration in `CommandLineParser`
Xrayez Aug 9, 2021
66e37de
Update docs for `CommandLineParser`
Xrayez Aug 9, 2021
7141f6e
Use `ERR_PARSE_ERROR` for `CommandLineParser`
Xrayez Aug 9, 2021
8b01b14
Merge `validate()` into `parse()` in `CommandLineParser`
Xrayez Aug 9, 2021
dcf414e
Add `new_option()` to `CommandLineParser`
Xrayez Aug 9, 2021
d6cb740
Change getter names to follow naming conventions in command line parser
Xrayez Aug 9, 2021
1a0fa2d
Use `index` over `idx` for CLI option arguments
Xrayez Aug 9, 2021
093b4d2
Declare `CommandLineParser` dependencies
Xrayez Aug 9, 2021
76a7e5c
Add more test cases for command line parser
Xrayez Aug 10, 2021
b66bbef
Rename `add_option()` to `append_option()`, `new_option()` to `add_op…
Xrayez Aug 10, 2021
05f4b44
Fix usage example of command-line parser
Xrayez Aug 10, 2021
1b77d73
Further improve docs for command-line parser
Xrayez Aug 11, 2021
b05da1d
Add prefix test cases for command-line parser
Xrayez Aug 11, 2021
514d73b
Fix `ParsedPrefix.is_exists()` name
Xrayez Aug 11, 2021
ff7ee37
Add important TODOs for Godot 4.0 regarding command-line parser
Xrayez Aug 11, 2021
1a41e9d
Add utility methods for `CommandLineOption`
Xrayez Aug 12, 2021
2eeadba
Validate disallowed default arguments in `CommandLineOption`
Xrayez Aug 12, 2021
8e4595c
Add test case for unrecognized options
Xrayez Aug 12, 2021
558dd3d
Move some variable closer to use in command-line parser
Xrayez Aug 12, 2021
37b7478
Mark `help` and `version` as special meta options
Xrayez Aug 12, 2021
f539979
Make sure meta option was parsed on the command-line
Xrayez Aug 12, 2021
a4859a6
Expose `meta` property in `CommandLineOption`
Xrayez Aug 12, 2021
20d3a30
Add positional options test case for command-line parser
Xrayez Aug 13, 2021
90ea98a
Simplify implementation of `CommandLineParser.add_option()` method
Xrayez Aug 13, 2021
9340536
Fix precedence of long prefix parsing in `CommandLineParser`
Xrayez Aug 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions core/command_line_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ void CommandLineOption::_bind_methods() {
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "required"), "set_required", "is_required");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "multitoken"), "set_multitoken", "is_multitoken");

ADD_SIGNAL(MethodInfo("validated", PropertyInfo(Variant::POOL_STRING_ARRAY, "values")));
ADD_SIGNAL(MethodInfo("parsed", PropertyInfo(Variant::POOL_STRING_ARRAY, "values")));
}

void CommandLineOption::set_names(const PoolStringArray &p_names) {
Expand Down Expand Up @@ -515,7 +515,6 @@ bool CommandLineParser::_contains_optional_options(const Vector<Pair<const Comma

void CommandLineParser::_bind_methods() {
ClassDB::bind_method(D_METHOD("parse", "args"), &CommandLineParser::parse);
ClassDB::bind_method(D_METHOD("validate"), &CommandLineParser::validate);

ClassDB::bind_method(D_METHOD("add_option", "option"), &CommandLineParser::add_option);
ClassDB::bind_method(D_METHOD("get_option_count"), &CommandLineParser::get_option_count);
Expand Down Expand Up @@ -619,10 +618,6 @@ Error CommandLineParser::parse(const PoolStringArray &p_args) {

_read_default_args();

return OK;
}

Error CommandLineParser::validate() {
for (int i = 0; i < _options.size(); ++i) {
Copy link
Contributor

@Shatur Shatur Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Xrayez Xrayez Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Shatur Shatur Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@Shatur Shatur Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Xrayez Xrayez Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Xrayez Xrayez Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const CommandLineOption *option = _options[i].ptr();
if (unlikely(option->is_required() && !_parsed_values.has(option))) {
Expand All @@ -634,7 +629,7 @@ Error CommandLineParser::validate() {
CommandLineOption *option = _options.get(i).ptr();
const Map<const CommandLineOption *, PoolStringArray>::Element *E = _parsed_values.find(option);
if (E) {
option->emit_signal("validated", E->value());
option->emit_signal("parsed", E->value());
}
}
return OK;
Expand Down
1 change: 0 additions & 1 deletion core/command_line_parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,6 @@ class CommandLineParser : public Reference {

public:
Error parse(const PoolStringArray &p_args);
Error validate();

void add_option(const Ref<CommandLineOption> &p_option);
int get_option_count() const;
Expand Down
6 changes: 3 additions & 3 deletions doc/CommandLineOption.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,14 @@
If [code]true[/code], option can be specified without a name. In this case, the first unparsed option marked as [member positional] will be selected.
</member>
<member name="required" type="bool" setter="set_required" getter="is_required" default="false">
If [code]true[/code], [method CommandLineParser.validate] will return an error if the option was not specified in the argument list.
If [code]true[/code], [method CommandLineParser.parse] will return an error if the option was not specified in the argument list.
</member>
</members>
<signals>
<signal name="validated">
<signal name="parsed">
<argument index="0" name="values" type="PoolStringArray" />
<description>
Emitted after calling [method CommandLineParser.validate] if it returns [constant OK] and this option was specified in the argument list. [code]values[/code] contains all values that were passed to the option.
Emitted after calling [method CommandLineParser.parse] if it returns [constant OK] and this option was specified in the argument list. [code]values[/code] contains all values that were passed to the option.
</description>
</signal>
</signals>
Expand Down
10 changes: 0 additions & 10 deletions doc/CommandLineParser.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,6 @@
print(ProjectSettings.get_setting("application/config/version"))
return

if parser.validate() != OK:
print(parser.get_error_text())
return

print(parser.get_value(input))
[/codeblock]
</description>
Expand Down Expand Up @@ -179,12 +175,6 @@
Replaces option at specified index [code]idx[/code] with [code]option[/code].
</description>
</method>
<method name="validate">
<return type="int" enum="Error" />
<description>
Checks if all required methods were specified. Emits [signal CommandLineOption.validated] upon success for options that were parsed.
</description>
</method>
</methods>
<members>
<member name="allow_adjacent" type="bool" setter="set_allow_adjacent" getter="is_allow_adjacent" default="true">
Expand Down
3 changes: 3 additions & 0 deletions tests/project/goost/core/test_command_line_parser.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ func test_parse():
var args = ["--input", "path"]

var input = CommandLineOption.new()
watch_signals(input)
input.names = ["input", "i"]

cmd.add_option(input)
Expand All @@ -21,6 +22,8 @@ func test_parse():
print(cmd.get_error_text())
assert_eq(err, OK)

assert_signal_emitted(input, "parsed")

var i = cmd.find_option("input")
assert_eq(input, i)

Expand Down