Softhouse / jargo

Argument and options parser for java
Other
17 stars 0 forks source link

Return Optional<T> unless defaultValue is supplied #33

Open adamretter opened 7 years ago

adamretter commented 7 years ago

I think the API would be much cleaner if ParsedArguments#get(Argument<T>) returned Optional<T> for parameters that had not been specified in the args or do not have a defaultValue.

This would also avoid the need to use ParsedArguments#wasGiven(Argument<T>).

For example:

import com.google.common.base.Optional;
import org.junit.Test;

import static org.fest.assertions.Assertions.assertThat;
import static se.softhouse.jargo.Arguments.stringArgument;

public class OptionalTest {

    @Test
    public void optional() {
        final Argument<Optional<String>> arg1 = stringArgument("s1")
                .description("string1")
                .build();

        final Argument<String> arg2 = stringArgument("s2")
                .description("string2")
                .defaultValue("hello")
                .build();

        final Argument<Optional<String>> arg3 = stringArgument("s3")
                .description("string3")
                .build();

        final ParsedArguments args = CommandLineParser
                .withArguments(arg1, arg2, arg3)
                .parse("--s3 this");

        assertThat(args.get(arg1)).isEqualTo(Optional.absent());
        assertThat(args.get(arg2)).isEqualTo("hello");
        assertThat(args.get(arg3)).isEqualTo(Optional.of("this"));
    }
}
jontejj commented 7 years ago

What's your use case?

My thoughts go like this: By default arguments are optional with a fallback to a default value, required arguments are the least popular scenario so therefore there is a .required() setting for those arguments. When it comes to optional arguments, I'm of the opinion that sane (and well documented) defaults are to prefer. If an api user insists on not having a value, they can use null as default value and checking if it was given by wasGiven, but this should be an edge case. That's why I thought this case shouldn't affect the default code path.

adamretter commented 7 years ago

In my use case, optional arguments are either present or omitted and it often does not make sense to give them default values.

As such I wrote this function to effectively give me a Optional like API that I could use:

public static <T> Optional<T> getOpt(final ParsedArguments parsedArguments, final Argument<T> argument) {
  if(parsedArguments.wasGiven(argument)) {
    return Optional.of(parsedArguments.get(argument));
  } else {
    return Optional.empty();
  }
}

I then added additional functions for some specific types: https://github.com/adamretter/exist/blob/args/src/org/exist/util/ArgumentUtil.java

The problem with setting default value to null is that it looks hideous in the output generated by --help.

You can see our use-cases with Jargo: 1) https://github.com/adamretter/exist/blob/args/src/org/exist/client/CommandlineOptions.java#L43 2) https://github.com/adamretter/exist/blob/args/src/org/exist/backup/ExportMain.java#L52 3) https://github.com/adamretter/exist/blob/args/src/org/exist/backup/Main.java#L78 4) https://github.com/adamretter/exist/blob/args/src/org/exist/jetty/ServerShutdown.java#L40 5) https://github.com/adamretter/exist/blob/args/src/org/exist/management/client/JMXClient.java#L250 6) https://github.com/adamretter/exist/blob/args/test/src/org/exist/performance/Main.java#L53 7) https://github.com/adamretter/exist/blob/args/test/src/xquery/TestRunnerMain.java#L59

jontejj commented 7 years ago

To develop our thoughts: If an argument is not given and a default value is not realistic, then I think the argument is more like a command? That is to say, the argument means that something more should be done. There's command support in jargo as well (http://softhouse.github.io/jargo/javadoc/jargo/se/softhouse/jargo/Command.html). Maybe you would benefit from grouping the arguments into commands? Or it's not possible to change your command line arguments due to legacy?

Note that the correct command order is important if there's a dependency between commands. I guess your use case makes sense if there's a dependency between a group of arguments but they are needed as a whole, need to think some more about this before implementing it...

By looking at the amount of parsers you have I think the command architecture makes sense :)

adamretter commented 7 years ago

@jontejj Indeed some of our args should have been modelled as commands. However I feel that we should remain backwards compatible for the users and any scripts and so we probably don't want to change the args.

I still think there is a good use-case for arguments that don't have default values. In that case an argument is both a flag and then if enabled, also configured with a user specified value.