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

Add support for passing through parameters to another command (was: How to handle find -exec kind-of open-ended options?) #718

Closed
rednoah opened this issue Jun 6, 2019 · 25 comments
Labels
theme: parser An issue or change related to the parser type: API 🔌 type: enhancement ✨
Milestone

Comments

@rednoah
Copy link

rednoah commented Jun 6, 2019

Is there a way to parse open-ended parameters that may contain - or -- up until some predefined end-of-parameter-sequence marker or the end of the argument array?

e.g. If I wanted to do something like find -exec, how would I go about it?

find . -type d -exec ls -la {} +
find . -type d -exec ls -la {} \;
  • where ls -la {} is an arbitrary command-template called for a given set of files by my application in a child process.
@remkop
Copy link
Owner

remkop commented Jun 6, 2019

You should be able to use setEndOfOptionsDelimiter.
I have added an example: https://github.com/remkop/picocli/blob/master/picocli-examples/src/main/java/picocli/examples/passthrough/Find.java

Running the example exposed a bug (thank you!) where the default end-of-options -- was used (instead of the specified end-of-options delimiter) to detect when to stop consuming arguments in an option with variable arity (arity = "*"). This bug is now fixed in master.

Can you take this example for a spin (ideally after building from master) and let me know if this meets your needs?

@rednoah
Copy link
Author

rednoah commented Jun 7, 2019

Sure thing. I'll report back on Monday!

@remkop
Copy link
Owner

remkop commented Jun 7, 2019

I created #719 for the end-of-options bug.

remkop added a commit that referenced this issue Jun 7, 2019
… on custom end-of-options delimiter

* the underlying issue was fixed in b426cee [#718]
* this commit adds a test and a release notes entry

Closes #719
@rednoah
Copy link
Author

rednoah commented Jun 8, 2019

Is there an API that would allow me to specify my own do my own argument parsing, akin to ITypeConverter, but that would also allow me to gobble up an arbitrary number of arguments?

e.g.

@Option(names = "-exec", arity = "1..*", arityHandler = "UntilPlusOrColon.class", description = "Execute command")
public List<String> exec;

e.g. akin to this API here:
https://github.com/kohsuke/args4j/blob/master/args4j/src/org/kohsuke/args4j/spi/OptionHandler.java#L66

@remkop
Copy link
Owner

remkop commented Jun 8, 2019

The idea behind ITypeConverter is to convert a single value, after which the converted value may be added to a collection of strongly typed (e.g. java.io.File) objects. There is no facility to have a single option that takes multiple arguments of different types.

Somehow I don't think that this is really what you want to do though.

Can you explain your use case in more detail? What is the problem you are trying to solve?

I can see that the -exec option wants to consume all subsequent arguments until it encounters a + or a ; argument.

  • If the problem is that you can only specify one end-of-options delimiter, and you need to support both + or a ;, then we can change picocli to accept multiple custom end-of-options delimiters. Would that solve the issue?
  • Is it valid input if the user specifies additional arguments after the + or ; argument? What do you want to do with the additional input? (In my example I suggest storing such arguments in the unmatched list of strings, but does that meet your requirements?)

@rednoah
Copy link
Author

rednoah commented Jun 8, 2019

  • If both + and ; were end-of-options delimiters, would I be able to know which one was encountered? As my -exec would have to behave different depending on whether there's a + or ; (or nothing) at the end.
  • Being able to do something like this -exec ls -la {} ';' file {} ';' --other-options would certainly be interesting. Saying -exec always has to be the last option is ok for me though, but it's a bit of a limitation.

@rednoah
Copy link
Author

rednoah commented Jun 8, 2019

Here's an example for my command-line tool usage:

e.g.

filebot -mediainfo . --filter f.audio -exec echo '{bitrate}' +
168732 161224 192000 128000

@remkop
Copy link
Owner

remkop commented Jun 8, 2019

Filebot is an impressive project!

Usually the end-of-options delimiter is consumed by the parser and the remainder is treated as positional parameters. If you need to distinguish between + and ; it may be best to simply capture everything in the parameters for -exec and process these parameters further in a second pass.

Happy to help try different approaches until we find something that meets your needs. I’ll try to experiment more with the examples you gave me so far. Do you have a branch with work in progress that I can look at?

If you have more examples of how users may invoke your tool and how you would like to process that input, that would be useful.

@rednoah
Copy link
Author

rednoah commented Jun 9, 2019

So bash_completion is the main feature that lead me to picocli, so I'm primarily using it for that right now:
https://github.com/filebot/filebot/blob/master/source/net/filebot/platform/bash/BashCompletionBuilder.java

I've been using args4j for many years and it's been working well, with the help of a few custom option handlers, so I'm not keen on changing to for production in the near future:
https://github.com/filebot/filebot/blob/master/source/net/filebot/cli/ArgumentBean.java

@rednoah
Copy link
Author

rednoah commented Jun 9, 2019

CLI options are a bit messy right now though, so I am thinking about a rewrite and reorganize with picocli in the future. But for now backwards compatibility is a concern, as to not break existing scripts that call filebot.

@remkop
Copy link
Owner

remkop commented Jun 11, 2019

Ok, interesting. The difference between arg4j's RestOfArgumentsHandler class and picocli's @Option(arity = "1..*") is that picocli will stop consuming arguments when it sees an argument that matches another option name in the command.

For example, your command has a -r option ("Recursively process folders"). If the end user specifies a command to -exec that also has -r as an option, that will cause picocli to stop assigning arguments to -exec. For example:

filebot -mediainfo . --filter f.audio -exec ls -r ;

In the above example, the only argument passed to the -exec List is ls, while we expect to get ls, -r and ;.

Passing through some of the arguments to a 3rd party command is an interesting use case and you are not the first to raise it. arg4j's RestOfArgumentsHandler idea may be a good way to handle this. I'm considering adding something similar to picocli.

Other than this, are there any other missing features or behaviour that differs from args4j that prevent you from migrating to picocli?

@rednoah
Copy link
Author

rednoah commented Jun 11, 2019

I think that was the only one that stuck out. I think the general use case here is some sort of interface that will allow one to use code to dynamically define arity based on the command-line values given, rather than statically.

e.g. I do have another custom handler though:

@Option(name = "--def", usage = "Define script variables", handler = BindingsHandler.class)
public Map<String, String> defines = new LinkedHashMap<String, String>();

So I can do:

--def a=1 b=2

As a less verbose way of doing:

--def a=1 --def b=2

I know that the verbose way is supported by picocli, but I'm not sure the short form. Maybe it's implicitly supported but I don't remember seeing anything in the examples. This one will just read args until the next -args is encountered, so List<String> would work here, but Map<String, String> makes more sense type-wise.

@remkop
Copy link
Owner

remkop commented Jun 11, 2019

You won't need a custom handler for map values in picocli. It provides what you need out of the box, and in addition you can have strongly typed keys and/or values. To illustrate:

@Test
public void testMapOptionArity1_n() {
    class MapParamsArity1_n {
        @Option(names = {"-D", "--def"}, arity = "1..*", split = ",")
        Map<String, Integer> params;
    }
    // most verbose
    MapParamsArity1_n params = CommandLine.populateCommand(
            new MapParamsArity1_n(), "--def", "a=1", "--def", "b=2", "--def", "c=3");
    Map<String, Integer> expected = new LinkedHashMap<String, Integer>();
    expected.put("a", 1);
    expected.put("b", 2);
    expected.put("c", 3);
    assertEquals(expected, params.params);

    // option name once, followed by values
    params = CommandLine.populateCommand(
            new MapParamsArity1_n(), "--def", "aa=11", "bb=22", "cc=33");
    expected.clear();
    expected.put("aa", 11);
    expected.put("bb", 22);
    expected.put("cc", 33);
    assertEquals(expected, params.params);

    // most compact (using the split regex)
    params = CommandLine.populateCommand(
            new MapParamsArity1_n(), "-Dx=4,y=5,z=6");
    expected.clear();
    expected.put("x", 4);
    expected.put("y", 5);
    expected.put("z", 6);
    assertEquals(expected, params.params);

    try {
        params = CommandLine.populateCommand(new MapParamsArity1_n(), "--def");
        fail("Should not accept input with missing parameter");
    } catch (MissingParameterException ex) {
        assertEquals("Missing required parameter for option '--def' at index 0 (<String=Integer>)",
                ex.getMessage());
    }
}

@rednoah
Copy link
Author

rednoah commented Jun 11, 2019

My bad. Looks like it works as expected right out of the box. :)

@remkop
Copy link
Owner

remkop commented Jun 11, 2019

Great, thanks for the confirmation.

About the custom argument handler, I'm thinking to introduce an interface like this:

/**
 * Options or positional parameters can be assigned a {@code IParameterConsumer} that implements
 * custom logic to process the parameters for this option or this position.
 * When an option or positional parameters with a custom {@code IParameterConsumer} is matched on the
 * command line, picocli's internal parser is temporarily suspended, and this object becomes
 * responsible for consuming and processing as many processing command line arguments as needed.
 * <p>This may be useful when passing through parameters to another command.</p>
 * <p>Example usage:</p>
 * <pre>
 * @Command(name = "find")
 * class Find {
 *     @Option(names = "-exec", parameterConsumer = Find.ExecParameterConsumer.class)
 *     List<String> list = new ArrayList<String>();
 *
 *     static class ExecParameterConsumer implements IParameterConsumer {
 *         public void consumeParameters(Stack<String> args, ArgSpec argSpec, CommandSpec commandSpec) {
 *             List<String> list = argSpec.getValue();
 *             while (!args.isEmpty()) {
 *                 String arg = args.pop();
 *                 list.add(arg);
 *
 *                 // `find -exec` semantics: stop processing after a ';' or '+' argument
 *                 if (";".equals(arg) || "+".equals(arg)) {
 *                     break;
 *                 }
 *             }
 *         }
 *     }
 * }</pre>
 * @see Option#parameterConsumer()
 * @see Parameters#parameterConsumer()
 * @since 4.0 */
public interface IParameterConsumer {
    /**
     * Consumes as many of the specified command line arguments as needed by popping them off
     * the specified Stack. Implementors are free to ignore the {@linkplain ArgSpec#arity() arity}
     * of the option or positional parameter, they are free to consume arguments that would
     * normally be matched as other options of the command, and they are free to consume
     * arguments that would normally be matched as an end-of-options delimiter.
     * <p>Implementors are responsible for saving the consumed values;
     * if the user object of the option or positional parameter is a Collection
     * or a Map, a common approach would be to obtain the current instance via the
     * {@link ArgSpec#getValue()}, and add to this instance. If the user object is an
     * array, the implementation would need to create a new array that contains the
     * old values as well as the newly consumed values, and store this array in the
     * user object via the {@link ArgSpec#setValue(Object)}.
     * </p><p>
     * If the user input is invalid, implementations should throw a {@link ParameterException}
     * with a message to display to the user.
     * </p><p>
     * When this method returns, the picocli parser will process the remaining arguments on the Stack.
     * </p>
     * @param args the command line arguments
     * @param argSpec the option or positional parameter for which to consume command line arguments
     * @param commandSpec the command that the option or positional parameter belongs to
     * @throws ParameterException if the user input is invalid
     */
    void consumeParameters(Stack<String> args, ArgSpec argSpec, CommandSpec commandSpec);
}

One issue that needs to be addressed is that the registry of type converters is currently private. This registry needs to be exposed as API, to allow IParameterHandler implementors to convert argument Strings to strongly typed values.

@rednoah
Copy link
Author

rednoah commented Jun 11, 2019

That was fast! Looks good to me. That should give very high level of flexibility for the people who need it. I'd just Stack.peek() and Stack.pop() to interpret the next few arguments, and use ArgSpec to set @Option fields accordingly.

@rednoah
Copy link
Author

rednoah commented Jun 12, 2019

Let me know if you have a revision where this is already included. Then I can try to build a sample for my -exec use case.

@remkop
Copy link
Owner

remkop commented Jun 12, 2019

Ok. I’ll try to push something to master in a few hours.

remkop added a commit that referenced this issue Jun 12, 2019
…Xxx.class)` attribute for passing arguments through to another command, like `find -exec`
@remkop remkop added this to the 4.0 milestone Jun 12, 2019
@remkop
Copy link
Owner

remkop commented Jun 12, 2019

This is now in master.
See this test for usage examples.

If you want to try this, it takes about 1 minute to build the project:

git clone https://github.com/remkop/picocli.git
cd picocli
./gradlew clean build

That will create the jar in build/libs/picocli-4.0.0-beta-2-SNAPSHOT.jar.

@remkop remkop changed the title How to handle find -exec kind-of open-ended options? Add support for passing through parameters to another command (was: How to handle find -exec kind-of open-ended options?) Jun 13, 2019
@remkop
Copy link
Owner

remkop commented Jun 13, 2019

Update: I renamed the interface to IParameterConsumer. This is more descriptive and avoids confusion with other interfaces whose names end in "Handler" that are completely unrelated.

@remkop
Copy link
Owner

remkop commented Jun 16, 2019

Closing this ticket, I think there is no more work left.
Feedback welcome!

@rednoah
Copy link
Author

rednoah commented Jun 16, 2019

Sure thing. I'll do some testing when time allows this week.

@remkop remkop modified the milestones: 4.0, 4.0-beta-2 Jun 18, 2019
remkop added a commit that referenced this issue Jun 19, 2019
@rednoah
Copy link
Author

rednoah commented Jun 20, 2019

Here's the solution I came up with:
https://gist.github.com/rednoah/9eae301e3ace6067807f1c58bb97b12c

@remkop
Copy link
Owner

remkop commented Jun 20, 2019

Nice! That looks pretty clean. 🎉 Let me know if you need anything else to migrate filebot to picocli.

FYI picocli 4.0.0-beta-2 has been released with this feature.

@rednoah
Copy link
Author

rednoah commented Jun 20, 2019

Nice, the new docs are simple and to the point.

@remkop remkop added the theme: parser An issue or change related to the parser label Apr 2, 2020
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: API 🔌 type: enhancement ✨
Projects
None yet
Development

No branches or pull requests

2 participants