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.

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

Repeated subcommands do not work correctly with programmatic API #1010

Open
remkop opened this issue Apr 26, 2020 · 1 comment
Open

Repeated subcommands do not work correctly with programmatic API #1010

remkop opened this issue Apr 26, 2020 · 1 comment
Labels
theme: parser An issue or change related to the parser type: bug 🐛

Comments

@remkop
Copy link
Owner

remkop commented Apr 26, 2020

This is a follow-up ticket to #1007.

That ticket fixed things so that repeatable subcommands correctly pick up any programmatic changes made via public CommandLine methods after the CommandLine instance was constructed.

What is remaining is that repeatable subcommands will not correctly pick up CommandSpec objects that were created programmatically or modified programmatically.

Applications that programmatically add subcommands or mixins, or build CommandSpec by programmatically creating and adding OptionSpec and PositionalParamSpec objects will not work correctly in repeatable subcommands.

@remkop remkop added type: bug 🐛 theme: parser An issue or change related to the parser labels Apr 26, 2020
@remkop remkop changed the title Repeated subcommands Repeated subcommands do not work correctly with programmatic API Apr 26, 2020
@remkop
Copy link
Owner Author

remkop commented Apr 26, 2020

To follow up on discussion with @diba1013 on #1007:

I'm assuming that you want to create a new copy to have a new command-object available to set the fields on, in order for it to be executed with the IParseResultHandler.

Does it make sense to split the configuration and internal representation from the state (which might need a larger refactoring and thus nothing to consider before 5.0)? I am thinking to postpone the population of the command fields/parameters just before the execution through the IParseResultHandler (i.e. into executeUserObject)? Some kind of Decorator could be useful, which is able to set the fields of the command-object or parameters in case of a command-method (thus holds the parsed values, similar to CommandSpec#argValues). The CommandLine then does not set the values directly anymore but saves the accessors within the Decorator. Then the IParseResultHandler would be responsible for populating the command (which might break some implementations?) and executing it afterwards.

I am basing this on code for picocli:4.2.0 as presented by the IDE, thus it might be out of date. Secondly, I am only skimming over the code and might not fully understand each implication.

I am still doing analysis to understand exactly how "deep" the copy of the CommandSpec needs to be. Currently, the model is wired together when it is being constructed. For example, if an @Option-annotated member is discovered, an OptionSpec object is constructed and wired in via the CommandSpec::addOption method. This registers the OptionSpec object in the command spec, but also registers the CommandSpec and its Messages (the i18n resource bundle) in the OptionSpec.

Each CommandSpec instance has its own command user object instance, which is the instance of the class with the @Option and @Parameters-annotated fields (glossing over some other usages here, but ok). A copy of the CommandSpec needs its own instance of the command user object instance. Then, we would like all OptionSpec objects in our copied CommandSpec to be rewired so that their getter and setter will retrieve/modify the field value from the command user object instance in the copy, not the command user object instance of the original CommandSpec.

There already exists a mechanism for this: there was a similar requirement for argument groups, which can also be repeatable. So, the ArgSpec (superclass of OptionSpec and PositionalParamSpec) has an IScope, which allows us to switch the command user object.

For @ArgGroups, the scope is set to a new group user object instance when needed.

For repeatable subcommands we would need to copy the OptionSpec and PositionalParamSpec objects and set their command user object instance via the same mechanism. Then, when an option is matched for the repeated subcommand, the annotated field of the new command user object instance is set.

There may be other details to iron out with copying options and positional parameters, but let's leave it at that for now.

Another item is subcommands and mixins. Subcommands have a reference to their parent command. So if we copy a CommandSpec, we may need to make a deep copy of the subcommands so that the subcommands in our copy point back to out copy, while the subcommands in our original CommandSpec point back to our original.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: parser An issue or change related to the parser type: bug 🐛
Projects
None yet
Development

No branches or pull requests

1 participant