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

Customize value assigned when option specified without value #280

Closed
pditommaso opened this issue Feb 5, 2018 · 41 comments
Closed

Customize value assigned when option specified without value #280

pditommaso opened this issue Feb 5, 2018 · 41 comments

Comments

@pditommaso
Copy link
Contributor

pditommaso commented Feb 5, 2018

The goal of the feature request is to provide the ability to specify a custom value to be used when a command line option is specified without any value.

See #279.

@pditommaso
Copy link
Contributor Author

Any chance to have this in 2.x release ?

@remkop
Copy link
Owner

remkop commented Feb 18, 2018

I have very little time so I need to focus on the 3.0 stream. I’m planning to only do 2.x releases if there’s a bug to be fixed.

For this feature request, I’m not ready to commit to any of the potential solutions I’ve seen/thought of so far. It often happens that feedback or questions from other users result in ideas for solving different existing tickets so (hoping that this will happen here also) I’m going to give this one some time.

My understanding is that the solution for #279 solves your immediate requirement so I didn’t think this was urgent. Is my understanding correct?

@pditommaso
Copy link
Contributor Author

Hi, that could work but I still feel it more like a workaround. I'm planning to migrate nextflow to picocli, but I would like to do that when it provides all features needed. However no hurry, just wanted to know what's the status of this, and thanks for this nice piece of software.

@pditommaso
Copy link
Contributor Author

Hi there, I was thinking something like this.

How that sounds to you ?

@remkop
Copy link
Owner

remkop commented Feb 28, 2018

I'm open to this in principle but I have a few concerns:

  • There is some work in progress (Allow Options with defaults to be always non-required #261) to add a defaultValue = "..." attribute to @Option and @Parameters. I think the proposed name implicit would be confusing. We need a name that makes it immediately obvious how this is different from the "normal" defaultValue attribute.
  • You are proposing a change against the 2.x branch. Given the limited time I can spend on picocli, I intended to update the 2.x branch for maintenance (bugfixes) only. New features should go into the master (3.0) branch. However, I would consider contributions to the 2.x branch (and potentially doing another 2.x release) if they are release-ready - so all I need to do is the release work. To me that means there are unit tests for the happy path as well as for any edge cases, the functionality is documented in the user manual and the release notes, and the same functionality is in the master (3.0) branch so there won't be a regression with the 3.0 release. This is quite a bit of work... :-)

@pditommaso
Copy link
Contributor Author

Hi, I've read #261 and frankly I've no understood what defaultValue brings more than assigning the default value to the field itself.

Anyhow in this case I think I will keep using the fork and wait to see how the project evolve.

@remkop
Copy link
Owner

remkop commented Feb 28, 2018

#261 raised a good point that options annotated with required=true should be non-mandatory if they have a default value. What prevented us from changing the parser behaviour to work like that is the realization that primitive fields always have a value.

So the idea was born to introduce a defaultValue attribute (and a defaultValueProvider dynamic alternative). The parser would not validate options marked as required = true when they have a default value (static or dynamic).

With the field default value it is not always possible to tell wether the field value was intended as a default or not.

@remkop
Copy link
Owner

remkop commented Feb 28, 2018

I've been thinking about a good name.

... I found these discussions for the same feature request in a different CLI parser:

Some possible names from that discussion: "tri-state option", "smart default", "optional argument for switches", "option without value"

Looking at your original description

the need for this is to allow the user to enable a feature and optionally to provide an extra parameter value.

Perhaps defaultSwitchValue is a good name that captures the essence that this is for "switch" options (where the presence or absence of the option itself is meaningful) as well as that this default value is separate from the "normal" defaultValue.

@pditommaso
Copy link
Contributor Author

pditommaso commented Feb 28, 2018

What prevented us from changing the parser behaviour to work like that is the realization that primitive fields always have a value.

Frankly, adding two extra settings (defaultValue and defaultValueProvider) just for this looks to me over-engineering.

I would just leave the user the choice to use primitive and non-primitive types accordingly:

  • primitive type field => implicitly optional
  • non-primitive type field w/o default => required (if marked as that)
  • non-primitive type field with default => implicitly optional

@remkop
Copy link
Owner

remkop commented Feb 28, 2018

Until recently I was reluctant to provide any picocli functionality for default values (see https://github.com/remkop/picocli/wiki/picocli-vs-JCommander#picocli-is-simpler-than-jcommander).

#261 convinced me that at least a defaultValueProvider attribute on @Command would be useful for applications that get default values from some external resource. The requester made a good point about how an option marked as required = true could become non-mandatory when a default value was obtained from such an external resource.

@jesselong
Copy link

For 2.x, would it be possible to pass an empty string to ITypeConverter when no value is present?

@remkop
Copy link
Owner

remkop commented Apr 5, 2018

Hi @jesselong, yes, #279 was included in picocli-2.3 so that is already available.

@jesselong
Copy link

@remkop Please see line 2541 which limits this to strings and prevents custom types getting an empty string in this case.

@remkop
Copy link
Owner

remkop commented Apr 6, 2018

@jesselong Ah yes, I see what you mean now. I will fix this on master so it will be in the next 3.0-alpha release, which will likely be soon. I am iterating rapidly with the 3.0 stream to get #258 ready in time for the Groovy 2.5 release. The 3.0 stream is my main focus now, and I won't be able to do a 2.3 release for this.

@pditommaso would Jesse's idea to use a custom converter for empty values meet your requirement? If not I will create another ticket to fix the problem Jesse pointed out, and keep this ticket open.

@pditommaso
Copy link
Contributor Author

Basically the idea is to use a ITypeConverter to map an empty string to a custom default value, right?

@remkop
Copy link
Owner

remkop commented Apr 6, 2018

Yes. Would that work for you, or should we keep this ticket open?

@pditommaso
Copy link
Contributor Author

It could be a workaround, but still thinking that a proper annotation would be a much better solution here.

@remkop
Copy link
Owner

remkop commented Apr 6, 2018

Ok, then we'll keep this ticket open. I created #325 for Jesse's suggestion.

@remkop
Copy link
Owner

remkop commented Sep 26, 2018

This feature turns out to be more popular than I thought: #490 is the 3rd time this is requested. Planning to implement this in picocli 3.7.

As for the name for this attribute, I'm currently thinking either implicit or valueWhenOmitted. Other ideas welcome.

@pditommaso
Copy link
Contributor Author

implicit +1, valueWhenOmitted sounds too verbose.

@remkop
Copy link
Owner

remkop commented Sep 26, 2018

Perhaps implicitValue; value can then be rendered in description with ${IMPLICIT-VALUE}.
This would be consistent with the existing defaultValue attribute and ${DEFAULT-VALUE} description variable.

@pditommaso
Copy link
Contributor Author

What's the difference between defaultValue and implictValue ?

@remkop
Copy link
Owner

remkop commented Sep 26, 2018

defaultValue is the value of the option when the option is not specified on the command line.
implicitValue is the value when the option name is specified without a value. (The latter is only relevant for options that have an optional parameter.)

A default value can be specified by

  • giving an annotated field an initial value in the declaration
  • the defaultValue = "value" attribute for annotated methods (option setter methods or command methods)

Examples:

// annotated field
@Option(names = "-f")
int field = 42;

// annotated method option 
@Option(names = "-m", defaultValue = "42")
public void setOption(String value) {
    // setter implementation (e.g. do option validation here)
}

// command method with method parameter option 
@Command
public void doit(@Option(names = "-c", defaultValue = "42") int c) {
    // command method implementation
}

@marinier
Copy link
Contributor

marinier commented Oct 1, 2018

The implicitValue functionality is related to a problem I have, which is to distinguish the cases when an option is not present, when the option is present but with no value, and the option is present with a value. implicitValue could be used to set a "special" value that means "present with no value". However, it rubs me the wrong way that we essentially need to use "special" values to represent the semantics of missing value. Perhaps we could introduce a new type, e.g., OptionalValue that acts like a tri-state optional. The default value would be "not present" (unless overridden by another specified default value or default value provider), and then a converter could set it to "present but no value" or set the actual value. implicitValue could still be used to assign an actual value if that makes sense for the application.

@remkop
Copy link
Owner

remkop commented Oct 1, 2018

Interesting idea to introduce a new type for this. Thinking out loud a bit to explore this...

Client code would look something like this:

class App implements Runnable {

  @Option(names = "-x")
  OptionalValue<Integer> x = OptionalValue.empty(); // value=null, implicitValue=null

  @Option(names = "-y")
  OptionalValue<Integer> y = OptionalValue.of(123);  // value=123, implicitValue=null

  @Option(names = "-z")
  OptionalValue<Integer> z =   // value=123, implicitValue=345
          OptionalValue.of(123).withImplicitValue(345);

  public void run() {
    if (x.isMatchedWithoutValue()) {
        doStuffWithImplicitValue(x.get());
    } else {
        doStuffWithSpecifiedValue(x.get());
    }
  }
}

OptionalValue would be an inner class of picocli.CommandLine and could look like this:

public static class OptionalValue<T> {
    private T value;
    private T implicitValue;
    private boolean matchedWithoutValue;

    private OptionalValue(T value) { this.value = value; }
    private OptionalValue(T value, T implicitValue) { 
        this.value = value; this.implicitValue = implicitValue; 
    }

    // invoked by picocli when parser matched option with a value,
    // or when option was annotated with @Option(defaultValue = "...")
    void matchedValue(T matchedValue) { value = matchedValue; }

    // invoked by picocli when parser matched option without value
    void matchedWithoutValue() { matchedWithoutValue = true; }

    // public API

    public static <T> OptionalValue<T> empty() { return of((T) null); }
    public static <T> OptionalValue<T> of(T value) { return new OptionalValue<T>(value); }

    public OptionalValue<T> withImplicitValue(T implicit) {
        return new OptionalValue<T>(this.value, implicit);
    }
    public boolean isMatchedWithoutValue() { return matchedWithoutValue; }
    public T get() { return matchedWithoutValue ? implicitValue : value; }
    public T getMatchedValue() { return value; }
    public T getImplicitValue() { return implicitValue; }
}

Thoughts?

@marinier
Copy link
Contributor

marinier commented Oct 3, 2018

This looks good to me! Does this interact with arity at all? E.g., would you need to say something like @Option(names = "-y", arity = "0..1") for this to make sense at all, or does using an OptionalValue imply an arity of 0..1? Also, would this be legal:

@Option(names = "-s", arity = "0..*")
OptionalValue<String[]> strings;

Not that it's important (to me) to support that in the first version if it's not easy.

@remkop
Copy link
Owner

remkop commented Oct 3, 2018

Very good point. The above OptionalValue implementation does not handle multi-value options.

In the current version of picocli (3.6.1), only single-value options allow you to distinguish between the option not being specified (value = default value) and option specified without a value (value is empty string).
For multi-value options, when the option is specified without a value, nothing happens.

This example demonstrates the current behaviour (without OptionalValue):

class Current implements Runnable {

    @Option(names = "-x", arity = "0..*")
    String single

    @Option(names = "-s", arity = "0..*")
    String[] strings

    @Option(names = "-l", arity = "0..*")
    List<String> list

    @Option(names = "-m", arity = "0..*")
    Map<String,String> map

    public static void main(String[] args) {
        println args
        CommandLine.run(new Current(), args)
    }

    @Override
    void run() {
        println "single value=$single"
        println "array=$strings"
        println "list=$list"
        println "map=$map"
    }
}

Current result:

[-x, -s, a, -s, b, c, -s, -s, d, e, f, -l, x, -l, -l, y, z, -m, a=b, -m, -m, c=d]
  ^                    ^                       ^                      ^
	
single value=
array=[a, b, c, d, e, f] // NOTE: array and collections have no empty string
list=[x, y, z]
map=[a:b, c:d]

Note that for multi-value options, when the option is specified without a value, simply nothing is added to the array/collection/map, so the application cannot detect whether the option was specified without a value.

With OptionalValue, the following should be possible:

class After implements Runnable {

    @Option(names = "-x", arity = "0..*")
    OptionalValue<String> single = OptionalValue.of(null).withImplicit("X")

    @Option(names = "-s", arity = "0..*")
    OptionalValue<String[]> strings = OptionalValue.of(null).withImplicit(new String[]{"S", "T"})

    @Option(names = "-l", arity = "0..*")
    OptionalValue<List<String>> list = OptionalValue.of(null).withImplicit(Arrays.asList("L", "M"))

    @Option(names = "-m", arity = "0..*")
    OptionalValue<Map<String,String>> strings = 
            OptionalValue.of(null).withImplicit(["KEY1":"VAL1", "KEY2":"VAL2"])

    public static void main(String[] args) {
        println args
        CommandLine.run(new After(), args)
    }

    @Override
    void run() {
        println "single value=$single"
        println "array=$strings"
        println "list=$list"
        println "map=$map"
    }
}

Expected result (for the same input):

[-x, -s, a, -s, b, c, -s, -s, d, e, f, -l, x, -l, -l, y, z, -m, a=b, -m, -m, c=d]
  ^                    ^                       ^                      ^
	
single value=X
array=[a, b, c, S, T, d, e, f]
list=[x, L, M, y, z]
map=[a:b, KEY1:VAL1, KEY2:VAL2, c:d]

Thoughts?

@pditommaso
Copy link
Contributor Author

For some aspect is tempting but thinking twice IMO it appears to me over engineered and would require the use of picocli domain classes in the application logic. It would be better to keep an annotation only approach.

I would take a simpler approach, add the implictValue attribute and clearly describe the difference in the docs.

Also it would be important to support the implicit flag for field having aritiy > 1.

@marinier
Copy link
Contributor

marinier commented Oct 4, 2018

In my view, this complexity is needed in this case -- as it is now, I have to inject the Spec object to extract this information, and was considering implementing something like OptionValue in my own project because it's cleaner. The annotation solution @pditommaso is suggesting relies on special values to indicate that an option is present with no value, which in my case is a hack (since I don't actually want a value at all).

Perhaps a compromise would be to support both: implicitValue as described by @pditommaso could be specified in the annotation for cases where that approach makes more sense, and OptionValue can be used where the implicitValue annotation is insufficient. (And it could be considered an error to specify both?)

@remkop That behavior for collections looks reasonable to me. To be clear, I don't have an immediate need for this ability, so if you'd rather leave it for a future release, that would be ok with me. But if it's not too difficult to include, it would be a more complete implementation.

@remkop
Copy link
Owner

remkop commented May 16, 2019

This issue has been sitting here too long, waiting for a decision... :-)
I plan to address this before the 4.0 GA release.

A simple annotation attribute (@Option(implicitValue = "..." or @Option(fallbackValue = "...") seems the best solution to me now.

The OptionValue idea is a powerful solution for the edge case where applications don't just need a value, but additionally need to know where the value came from. The problem is that by offering such first-class citizen support for this edge case, casual users may get confused into thinking that this is the recommended or idiomatic usage.

Applications that do need to know this can find out via the @Spec annotation or by selecting a special value. This makes the easy things easy, and the hard things possible...

About naming: I've looked at prior art and found very little, but lisp CLON supports this feature and calls it the fallback-value. I like this name slightly better than implicitValue.

From the CLON documentation on value sources:

  • Fallback values are used when an option exists on the command-line without a corresponding argument (so this applies only to options taking optional arguments only).

Difference with CLON: in CLON, when an option’s argument is optional, the option is required to provide at least a fallback or a default value. If no fallback value is specified the default value is assigned.

Picocli will behave differently:

  • if a value is specified on the command line, this value is used
  • if the option is specified without option parameter, and the option parameter is optional, the fallback value is assigned. If no fallback value is specified, an empty String is assigned. (This is for backwards compatibility with previous picocli versions.)
  • if the option is not specified on the command line, the default value is assigned.

I will also take a look at what is involved in assigning the fallback value for multi-value options with arity = "0..n" when the option is specified without parameters.

@remkop remkop modified the milestones: 4.1, 4.0 May 16, 2019
@marinier
Copy link
Contributor

This sounds good to me!

remkop added a commit that referenced this issue Jun 11, 2019
…gn this value when the option was specified on the command line without parameter.
@remkop
Copy link
Owner

remkop commented Jun 11, 2019

This is now in master. Please verify whether this meets your needs. The user manual is updated here and here.

From the release notes:

fallbackValue API

This release introduces a new attribute on the Option annotation: fallbackValue for options with optional parameter: assign this value when the option was specified on the command line without parameter.

This is different from the defaultValue, which is assigned if the option is not specified at all on the command line.

Using a fallbackValue allows applications to distinguish between cases where

  • the option was not specified on the command line (default value assigned)
  • the option was specified without parameter on the command line (fallback value assigned)
  • the option was specified with parameter on the command line (command line argument value assigned)

This is useful to define options that can function as a boolean "switch" and optionally allow users to provide a (strongly typed) extra parameter value.

The option description may contain the ${FALLBACK-VALUE} variable which will be replaced with the actual fallback value when the usage help is shown.

@remkop
Copy link
Owner

remkop commented Jun 11, 2019

I also added support for multi-value options (array, List and Map type annotated elements).

@remkop
Copy link
Owner

remkop commented Jun 11, 2019

@pditommaso , @marinier it would be great if you could verify that this meets your requirements.

@marinier
Copy link
Contributor

This looks great. Thank you!

@remkop remkop modified the milestones: 4.0, 4.0-beta-2 Jun 18, 2019
@remkop
Copy link
Owner

remkop commented Jun 19, 2019

@pditommaso, @jesselong, @marinier
picocli 4.0.0-beta-2 has been released 🎉 with support for the fallbackValue attribute for options with an optional parameter.

Sorry it took so long. 😓

@remkop
Copy link
Owner

remkop commented Aug 15, 2019

@pditommaso picocli 4.0 GA was released in July with support for this feature:
Options can have a fallbackValue attribute that is used when the option is specified on the command line without a value. The user manual has an example.

There's also a bunch of other new features in picocli 4.0 that may be of interest: mutually exclusive options, co-dependent options, repeating argument groups, variable expansion, a cleaner API for executing commands with better exit code support, and more.

If there are any other blockers to migrating the Nextflow CLI to picocli, please let me know.

@pditommaso
Copy link
Contributor Author

Hi Remko, thanks for keeping me updated on this. I'll explore these new features at my earliest convenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants