Add command-line parser class #44594

Shatur · 2020-12-22T20:07:54Z

Continuation of #26213.

Thank you a lot to @lupoDharkael for the initial work! I updated the code to the latest master and heavily redone the previous implementation. What I added:

Usage generation (can be disabled).
Support for positional arguments (it's better to support it instead of manually checking for scene / script).
Multiply arguments support.
Support for default values.
Support for required arguments (checked automatically after calling notify())
Support for compound arguments (-abc), sticky arguments (-dvalue), adjacent arguments (-d=value) (all this things can be disabled).
The ability to specify allowed values lists (syntactic sugar for checkers).
Support for any number of flags in a single command.
Support for use with different prefixes (e.g. /command, +command, -command, etc, this can be useful for chat commands parsing, for example).
Custom actions on parsing via parsed signal.
Support for arguments that can be specified multiply times.
The ability to configure help message padding / size.
Tests.
General API improvements.

Simple demo project: TestParser.zip

Current API looks similar to Qt (API) and Boost.program_options (features).

But I haven't made any changes to main.cpp. I think it will be better to agree on API first and merge it as a class for GDScript because it can be used as is. Then, I will send a separate PR with changes to main.cpp and then another PR with the ability to disable engine arguments to override with custom one. I think that this approach will simplify review process.

Shatur · 2020-12-22T20:21:46Z

After adding tests from me, test_main.cpp got too big and can't compile on Windows without /bigobj. Could anyone help me with this? I am not familiar with SCons.

Xrayez · 2020-12-22T23:02:21Z

After adding tests from me, test_main.cpp got too big and can't compile on Windows without /bigobj. Could anyone help me with this? I am not familiar with SCons.

Perhaps somehow related to template fixture usage which may bloat it? I haven't done this to be honest...

Possibly /bigobj could be added specifically for tests env in SCons to resolve this (in https://github.com/godotengine/godot/blob/master/tests/SCsub).

Shatur · 2020-12-23T09:23:37Z

Perhaps somehow related to template fixture usage which may bloat it? I haven't done this to be honest...

It seems to me that the file was large before that, and adding a lot of new tests just exceeded the maximum size allowed.

Possibly /bigobj could be added specifically for tests env in SCons to resolve this (in https://github.com/godotengine/godot/blob/master/tests/SCsub).

Thanks, it worked :)

tests/SCsub

Shatur · 2021-04-20T17:25:22Z

@akien-mga, I noticed that you changed the description. But this PR is not a refactoring of the current editor CLI.
I thought to provide a test-covered class that can be used in GDScript with this PR, and then use it to refactor the engine command line arguments parsing in another PR. Just to simplify review process and avoid merge conflicts. What do you think about it?
Also this PR is ready for review, just waiting for your feedback :)

stijn-h · 2021-05-29T08:28:24Z

This PR is not a refactoring of the current editor CLI. I thought to provide a test-covered class that can be used in GDScript with this PR, and then use it to refactor the engine command line arguments parsing in another PR.

I have a feeling this PR is not going to get looked at by the devs if it doesn't make an attempt to refactor the Godot CLI. Right now, this PR has no use case other than exposure in the API, something only a few people have shown a little interest for. Refactoring main.cpp is (in my understanding) a more important goal. The initial proposal (godotengine/godot-proposals#137) follows this line of thinking as well.

As it stands, this PR does not implement that proposal, and as no real need for an exposed command parser has been explicitely requested. That leads me to believe the devs will ignore your work, which is a shame because it looks promising. If you haven't already, take a look at Godot's best practices for contributing.

Xrayez · 2021-05-29T10:26:12Z

Refactoring main.cpp is (in my understanding) a more important goal.

While it's an important goal, adding a general-purpose command-line parser is a crucial step to achieve it, even if we don't end up exposing it as public API.

As it stands, this PR does not implement that proposal, and as no real need for an exposed command parser has been explicitely requested.

I'd definitely find the parser useful for a lot of things. Plugins like GUT would no longer need to implement their own command-line parsers. Game-specific options could also be implemented more easily: godotengine/godot-proposals#2726.

Even Python has argparse module, and GDScript is known to be similar to Python. The fact that Godot can execute scripts with --script parameter only supports the idea that we do have use cases that could be solved with this.

stijn-h · 2021-05-29T10:51:26Z

While it's an important goal, adding a general-purpose command-line parser is a crucial step to achieve it, even if we don't end up exposing it as public API.

I don't disagree, in fact I believe this is what I said as well. All I am saying is that I think this PR should define a command line parser and refactor main.cpp to use it. Optionally it could be exposed to the api already, but that can also happen in another PR.

Xrayez

The functionality of this class is promising, but I think we should agree upon API and naming conventions to move forward first.

CommandParser and CommandOption could be renamed to CommandLineParser and CommandLineOption respectively, to be more explicit and avoid ambiguity with Command pattern in programming in general. It's more explicit and goes in accordance to OS.get_cmdline_args() method.

Regarding naming conventions, I think it would be beneficial to rename command parameters to option in general. I'd use "option" because that's a noun which should be taken as a base for the entire API. So, instead of add_command(CommandLineOption p_command), it should be add_option(CommandLineOption p_option), etc.

Help formatting properties could be refactored into Dictionary, to prevent API bloat in the parser class.

To make it useful for game-specific needs and make it more configurable, it may also be worth to implement a virtual _get_help_text(format_options: Dictionary) function which can be overridden via script/C++. Implementing a virtual method can also alleviate the problem of having formatting properties hardcoded directly in the class.

Don't forget to go through hidden conversations, because GitHub didn't like the fact that I have left so many review comments...

I haven't reviewed the documentation yet (there are typos which can be fixed, though).

core/command_parser.h

Xrayez · 2021-05-29T11:52:34Z

core/command_parser.h

+	void set_static_checker(CheckFunction p_function, const String &p_error_msg);
+	Error set_checker(const Callable &p_callable, const String &p_error_msg);
+	const ArgumentChecker *get_checker() const;
+	void remove_checker();


Not sure if really needed, can just do:

set_checker(null)

But we have set_static_checker (for use in C++ only) and set_checker helpers to create ArgumentChecker. ArgumentChecker is not exported to GDScript. I wanted to provide a similar interface to connect / disconnect.

core/command_parser.h

core/command_parser.cpp

core/command_parser.h

Xrayez · 2021-05-29T12:31:21Z

@akien-mga, I noticed that you changed the description. But this PR is not a refactoring of the current editor CLI.

I have renamed the PR's title to reflect changes in this PR. Yeah this doesn't refactor the main.cpp argument parsing in the engine, and that could be made in the successive PRs.

Xrayez · 2021-05-29T13:01:07Z

While it's an important goal, adding a general-purpose command-line parser is a crucial step to achieve it, even if we don't end up exposing it as public API.

I don't disagree, in fact I believe this is what I said as well. All I am saying is that I think this PR should define a command line parser and refactor main.cpp to use it. Optionally it could be exposed to the api already, but that can also happen in another PR.

I don't disagree either, but exposed or not, the work done on refactoring main.cpp could be in vain if we end up not agreeing with the public (or rather internal, depending on what you treat as internal) API for the parser (and this is prominent by above reviews of mine), especially when this has a chance to be exposed to scripting.

I myself prefer to look into the future a bit, but I realize that Godot's development is extremely pragmatic so I'm not sure whether what I said even makes sense in terms of Godot's development principles. 😛

stijn-h · 2021-05-29T13:38:33Z

Exposed or not, the work done on refactoring main.cpp could be in vain if we end up not agreeing with the public (or rather internal, depending on what you treat as internal) API for the parser (and this is prominent by above reviews of mine), especially when this has a chance to be exposed to scripting.

Comes back to the point of what you prioritise: engine or api. I am fine either way, whichever is preferred by the maintainers. I just think if you develop the parser first for the engine, you have a clear use case where you can judge the implementation. That can then help with designing the public API.

Xrayez · 2021-05-29T13:47:45Z

I just think if you develop the parser first for the engine, you have a clear use case where you can judge the implementation. That can then help with designing the public API.

The previous PR #26213 did refactoring but ended up not reviewed either. Since this PR is based on #26213, I'd assume that it is already somewhat battle-tested against use cases in Godot (which are not that advanced, to be honest), and the implementation in this PR is by far more flexible than Godot really needs.

Unfortunately, this may be a problem to adoption, because as I said, Godot's development is extremely pragmatic and this PR may also end up in Limbo, just because it solves more problems than Godot currently has, which is pity to be honest... Because I think it does make sense to make functionality feature-complete, regardless whether we think it will be used often or not.

I'm personally interested in such a class and it would be useful to have in core. However, if this won't happen, perhaps this can be implemented in Goost.

reduz · 2021-05-29T14:34:08Z

Indeed, as @Xrayez mentions, there is not a proper proposal opened following proper procedure that can justify in real-world usage scenario integrating this kind of functionality. As such this PR will most likely end up in limbo. I suggest re-starting the process from scratch, opening a proper proposal and seeing if there is any interest.

reduz · 2021-05-29T15:09:08Z

I opened a counter-proposal about this:

godotengine/godot-proposals#2797

Shatur · 2021-05-30T11:35:25Z

@Xrayez, thank you a lot, will push discussed changes in a few days. I'm going to move this PR to Goost, if the developers don't change their minds. Have this class as a plugin sounds good to me too.

Xrayez

The exposed API looks much better to me now. As a user, I find it easier to figure out what can be done more on an intuitive level.

Again, haven't reviewed documentation because I think we could wait for Akien's planned review as well before suggesting documentation changes.

core/command_line_parser.h

core/command_line_parser.cpp

core/command_line_parser.h

Shatur · 2021-06-01T22:11:41Z

@Xrayez, thanks, yes, I pretty sure that the documentation should be improved.

Port of godotengine/godot#44594 to Godot 3.x. Co-authored-by: Shatur95 <[email protected]> Co-authored-by: lupoDharkael <[email protected]>

Xrayez · 2021-08-08T14:18:54Z

@Shatur I've started working on porting this PR to Goost in Godot 3.x as we've discussed at Godot contributors chat, see goostengine/goost#123.

I'm writing test cases in GDScript and noticed that the following test case:

func test_parse():
    var args = ["--input path"]

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

    parser.add_option(input)
    var err = parser.parse(args)
    if err:
        print(parser.get_error_text())
    assert_eq(err, OK)

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

    assert_eq(parser.get_value(i), "path")

fails and prints:

res://goost/core/test_command_line_parser.gd
* test_parse
'--input path' is not a valid option.
Maybe you wanted to use: '--input'.
    [Failed]:  [30] expected to equal [0]:
      at line -1
    [Failed]:  [""] expected to equal ["path"]:
      at line -1
* test_builtin_options

According to documentation, both = and (space) are supported. Is it a bug or am I missing something?

EDIT: nevermind, I've forgot again that args should be passed as a list of strings, not a single string. I wonder what could be done to avoid this mistake, as command-line arguments could be technically parsed from a single string, not just OS.get_cmdline_args(), which already splits the command-line string for us.

P.S. For those wondering why we've decided to port this PR to Goost: reduz already rejected the idea of having a general-purpose parser to be in core, see godotengine/godot-proposals#2797 (comment).

Shatur · 2021-08-08T16:14:21Z

I wonder what could be done to avoid this mistake

I have no idea :) In theory we could have two methods, one for a single string, second for a list of strings.

Xrayez · 2021-08-08T16:16:48Z

I have no idea :) In theory we could have two methods, one for a single string, second for a list of strings.

I think parse method could take Variant over PackedStringArray, and cast variant to either String or PackedStringArray, depending on Variant.get_type().

Shatur · 2021-08-08T16:20:51Z

I think parse method could take Variant over PackedStringArray

I think it can confuse users a little :)

Port of godotengine/godot#44594 to Godot 3.x. Co-authored-by: Shatur95 <[email protected]> Co-authored-by: lupoDharkael <[email protected]>

Xrayez · 2021-08-11T18:32:34Z

I have discovered that prefix precedence in long_prefixes/short_prefixes matters:

func test_get_prefix_precedence():
    var debug = CommandLineOption.new()
    debug.names = ["debug"]
    debug.arg_count = 0
    parser.add_option(debug)

    parser.long_prefixes = ["--no-", "--"]

    assert_eq(parser.parse_args(["--debug"]), OK)
    assert_true(parser.is_set(debug))
    assert_eq(parser.get_prefix(debug), "--")

    assert_eq(parser.parse_args(["--no-debug"]), OK)
    assert_true(parser.is_set(debug))
    assert_eq(parser.get_prefix(debug), "--no-")
    
    parser.long_prefixes = ["--", "--no-"] # Swap precedence.

    assert_eq(parser.parse_args(["--debug"]), OK)
    assert_true(parser.is_set(debug))
    assert_eq(parser.get_prefix(debug), "--")

    assert_eq(parser.parse_args(["--no-debug"]), ERR_PARSE_ERROR,
            "Precedence of `--` prefix is higher than `--no-`, should fail.")
    assert_false(parser.is_set(debug))

Note that both -- and --no- prefixes share -- characters.

The above test case works, but I wonder if that's expected. Should this rely on precedence of prefixes?

Shatur · 2021-08-11T19:38:29Z

The above test case works, but I wonder if that's expected. Should this rely on precedence of prefixes?

No, I think it can be fixed. We should check for each prefix before return the error.

Xrayez · 2021-08-12T12:29:43Z

Found another test case, looks like a bug to me:

func test_default_not_allowed_arg():
    var filetype = CommandLineOption.new()
    filetype.names = ["filetype"]
    filetype.default_args = ["png"]
    filetype.allowed_args = ["jpg"]

    parser.add_option(filetype)

    assert_eq(parser.parse_args([]), ERR_PARSE_ERROR, "Default argument `png` is not allowed one, should fail.")
    assert_eq(parser.get_value(filetype), "")

Note that this works as expected when you parse --filetype png, and get_error_text() will contain:

Argument 'png' cannot be used for '--filetype', possible values: jpg.

I think parsing should fail immediately if default argument is not allowed according to allowed_args.

Shatur · 2021-08-12T12:57:33Z

I think parsing should fail immediately if default argument is not allowed according to allowed_args.

Agree, we should have a check for it.

Xrayez · 2021-08-12T13:11:16Z

I think parsing should fail immediately if default argument is not allowed according to allowed_args.

Agree, we should have a check for it.

Fixed in 2eeadba (#123).

Xrayez · 2021-08-13T12:17:40Z

The above test case works, but I wonder if that's expected. Should this rely on precedence of prefixes?

No, I think it can be fixed. We should check for each prefix before return the error.

Fixed in 9340536 (#123).

goostengine/goost#123 is now merged in Goost as well.

Xrayez · 2021-08-14T12:01:33Z

Found a test case which crashes the engine, see goostengine/goost#125 for reproduction and solution.

Shatur · 2021-08-14T13:03:01Z

Found a test case which crashes the engine, see goostengine/goost#125 for reproduction and solution.

Nice catch!

Shatur requested a review from a team as a code owner December 22, 2020 20:07

Shatur mentioned this pull request Dec 22, 2020

[WIP] Add command parser #26213

Closed

4 tasks

Calinou added enhancement topic:core labels Dec 22, 2020

Calinou added this to the 4.0 milestone Dec 22, 2020

akien-mga self-requested a review December 22, 2020 23:02

Xrayez reviewed Dec 30, 2020

View reviewed changes

tests/SCsub Outdated Show resolved Hide resolved

Shatur removed the request for review from a team December 30, 2020 16:17

akien-mga changed the title ~~Add CommandParser~~ Refactor command line arguments parsing with a dedicated CommandParser Apr 6, 2021

akien-mga mentioned this pull request Apr 6, 2021

Main: Default --doctool path to '.' if none given #47647

Merged

Calinou mentioned this pull request May 15, 2021

-w switch is not in effect when opening projects from the project manager #3070

Open

Xrayez reviewed May 29, 2021

View reviewed changes

Xrayez changed the title ~~Refactor command line arguments parsing with a dedicated CommandParser~~ Add command-line parser class May 29, 2021

Shatur mentioned this pull request May 29, 2021

Separate command line arguments from engine and game/editor godotengine/godot-proposals#2797

Closed

Shatur requested review from a team as code owners June 1, 2021 15:59

Shatur requested a review from a team as a code owner June 1, 2021 20:28

Xrayez reviewed Jun 1, 2021

View reviewed changes

core/command_line_parser.h Outdated Show resolved Hide resolved

core/command_line_parser.cpp Outdated Show resolved Hide resolved

core/command_line_parser.h Outdated Show resolved Hide resolved

Add CommandLineParser

7e325ed

Xrayez added a commit to goostengine/goost that referenced this pull request Aug 7, 2021

Add command-line parser

787f13e

Port of godotengine/godot#44594 to Godot 3.x. Co-authored-by: Shatur95 <[email protected]> Co-authored-by: lupoDharkael <[email protected]>

Xrayez added a commit to goostengine/goost that referenced this pull request Aug 8, 2021

Add command-line parser

6a1710d

Port of godotengine/godot#44594 to Godot 3.x. Co-authored-by: Shatur95 <[email protected]> Co-authored-by: lupoDharkael <[email protected]>

Calinou mentioned this pull request Aug 8, 2021

[TRACKER] Command line-related issues #51393

Open

Xrayez added a commit to goostengine/goost that referenced this pull request Aug 9, 2021

Add command-line parser

2cd203a

Port of godotengine/godot#44594 to Godot 3.x. Co-authored-by: Shatur95 <[email protected]> Co-authored-by: lupoDharkael <[email protected]>

Xrayez added a commit to goostengine/goost that referenced this pull request Aug 9, 2021

Add command-line parser

d7d64e2

Port of godotengine/godot#44594 to Godot 3.x. Co-authored-by: Shatur95 <[email protected]> Co-authored-by: lupoDharkael <[email protected]>

Shatur mentioned this pull request Aug 14, 2021

Refactor main.cpp setup #49362

Draft

YuriSizov modified the milestones: 4.0, 4.x Feb 9, 2023

neikeq mentioned this pull request Mar 8, 2023

Prevent crashing on startup if project has scripted theme types #74565

Merged

Calinou mentioned this pull request Apr 1, 2023

Improve readability of main.cpp. #75548

Open

Calinou mentioned this pull request Aug 20, 2023

Add support for --argument=value syntax in engine command line arguments (in addition to --argument value) godotengine/godot-proposals#7535

Open

Shatur closed this by deleting the head repository Mar 13, 2024

AThousandShips added the archived label Mar 13, 2024

AThousandShips removed this from the 4.x milestone Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add command-line parser class #44594

Add command-line parser class #44594

Shatur commented Dec 22, 2020 •

edited

Loading

Shatur commented Dec 22, 2020 •

edited

Loading

Xrayez commented Dec 22, 2020 •

edited

Loading

Shatur commented Dec 23, 2020 •

edited

Loading

Shatur commented Apr 20, 2021 •

edited

Loading

stijn-h commented May 29, 2021

Xrayez commented May 29, 2021

stijn-h commented May 29, 2021

Xrayez left a comment •

edited

Loading

Xrayez May 29, 2021

Shatur May 30, 2021

Xrayez commented May 29, 2021

Xrayez commented May 29, 2021 •

edited

Loading

stijn-h commented May 29, 2021

Xrayez commented May 29, 2021 •

edited

Loading

reduz commented May 29, 2021

reduz commented May 29, 2021

Shatur commented May 30, 2021

Xrayez left a comment •

edited

Loading

Shatur commented Jun 1, 2021

Xrayez commented Aug 8, 2021 •

edited

Loading

Shatur commented Aug 8, 2021

Xrayez commented Aug 8, 2021

Shatur commented Aug 8, 2021

Xrayez commented Aug 11, 2021 •

edited

Loading

Shatur commented Aug 11, 2021

Xrayez commented Aug 12, 2021

Shatur commented Aug 12, 2021

Xrayez commented Aug 12, 2021

Xrayez commented Aug 13, 2021 •

edited

Loading

Xrayez commented Aug 14, 2021

Shatur commented Aug 14, 2021

Add command-line parser class #44594

Add command-line parser class #44594

Conversation

Shatur commented Dec 22, 2020 • edited Loading

Shatur commented Dec 22, 2020 • edited Loading

Xrayez commented Dec 22, 2020 • edited Loading

Shatur commented Dec 23, 2020 • edited Loading

Shatur commented Apr 20, 2021 • edited Loading

stijn-h commented May 29, 2021

Xrayez commented May 29, 2021

stijn-h commented May 29, 2021

Xrayez left a comment • edited Loading

Choose a reason for hiding this comment

Xrayez May 29, 2021

Choose a reason for hiding this comment

Shatur May 30, 2021

Choose a reason for hiding this comment

Xrayez commented May 29, 2021

Xrayez commented May 29, 2021 • edited Loading

stijn-h commented May 29, 2021

Xrayez commented May 29, 2021 • edited Loading

reduz commented May 29, 2021

reduz commented May 29, 2021

Shatur commented May 30, 2021

Xrayez left a comment • edited Loading

Choose a reason for hiding this comment

Shatur commented Jun 1, 2021

Xrayez commented Aug 8, 2021 • edited Loading

Shatur commented Aug 8, 2021

Xrayez commented Aug 8, 2021

Shatur commented Aug 8, 2021

Xrayez commented Aug 11, 2021 • edited Loading

Shatur commented Aug 11, 2021

Xrayez commented Aug 12, 2021

Shatur commented Aug 12, 2021

Xrayez commented Aug 12, 2021

Xrayez commented Aug 13, 2021 • edited Loading

Xrayez commented Aug 14, 2021

Shatur commented Aug 14, 2021

Shatur commented Dec 22, 2020 •

edited

Loading

Shatur commented Dec 22, 2020 •

edited

Loading

Xrayez commented Dec 22, 2020 •

edited

Loading

Shatur commented Dec 23, 2020 •

edited

Loading

Shatur commented Apr 20, 2021 •

edited

Loading

Xrayez left a comment •

edited

Loading

Xrayez commented May 29, 2021 •

edited

Loading

Xrayez commented May 29, 2021 •

edited

Loading

Xrayez left a comment •

edited

Loading

Xrayez commented Aug 8, 2021 •

edited

Loading

Xrayez commented Aug 11, 2021 •

edited

Loading

Xrayez commented Aug 13, 2021 •

edited

Loading