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

behavior of defaultValue #962

Closed
sjyMystery opened this issue Feb 20, 2020 · 8 comments
Closed

behavior of defaultValue #962

sjyMystery opened this issue Feb 20, 2020 · 8 comments

Comments

@sjyMystery
Copy link

sjyMystery commented Feb 20, 2020

Hi, guys:
There's an unexpected behavior of the defaultValue param in @Option Annotation.
When use default values from environment variables of invalid types, it always throws ParameterException even though I passed the value of the argument.

How to solve it

Maybe we can disable the type-checking of default value when the argument value has been already passed.

And I will be honored to submit the example code of this situation and create a pull request if necessary.

@remkop
Copy link
Owner

remkop commented Feb 20, 2020

Can you please show how to reproduce this problem?

@sjyMystery
Copy link
Author

Of course, the code is below:

import picocli.CommandLine;

@CommandLine.Command(name = "run")
public class App implements Runnable {

    @CommandLine.Option(names = "--port", defaultValue = "${env:PORT}", required = true)
    Integer PORT;


    static public void main(String[] args) {
        App app = new App();
        args = new String[]{"--port", "5672"};
        CommandLine cmd = new CommandLine(app);
        cmd.execute(args);
    }

    public void run() {
        System.out.println(PORT);

    }
}

before run :

export PORT="xxx"

@sjyMystery
Copy link
Author

sjyMystery commented Feb 20, 2020

then may below information in stderr:

Invalid value for option '--port': 'xxx' is not an int
Usage: run [--port=PORT>]
      --port=<PORT>

@remkop
Copy link
Owner

remkop commented Feb 20, 2020

@sjyMystery Thank you for the clarification. I understand the issue now.
It looks like this is the same cause as this ticket: #961 (I just changed the title of that ticket.)

Please read my comment on that ticket for more details.

Are you interested in providing a pull request for this? That would be great!

@sjyMystery
Copy link
Author

sjyMystery commented Feb 21, 2020

@remkop I've get your point.By the way, is there anything need or suggested to read before I make a pr?Thanks.

@remkop
Copy link
Owner

remkop commented Feb 21, 2020

@sjyMystery There is no documentation on the implementation, just the source code. :-)

It may be as simple as reordering these 2 lines in CommandLine$Interpreter.parse(List, Stack, String[], List) on line 11486:

Before:

private void parse(List<CommandLine> parsedCommands, Stack<String> argumentStack, String[] originalArgs, List<Object> nowProcessing) {
...
  try {
    applyDefaultValues(required); // currently defaults are applied first
    processArguments(parsedCommands, argumentStack, required, initialized, originalArgs, nowProcessing);
  } catch (InitializationException ex) {
...

After:

private void parse(List<CommandLine> parsedCommands, Stack<String> argumentStack, String[] originalArgs, List<Object> nowProcessing) {
...
  try {
    processArguments(parsedCommands, argumentStack, required, initialized, originalArgs, nowProcessing);
    applyDefaultValues(required, initialized);  // only apply defaults to the missing args
  } catch (InitializationException ex) {
...

Then, in applyDefaultValues(), we only modify the ArgSpec objects that are not in the initialized set.

But let's add some tests to verify.

@remkop
Copy link
Owner

remkop commented Mar 7, 2020

@sjyMystery Have you had a chance to look at this? Let me know if you get stuck anywhere, I will try to help. :-)

@remkop remkop added this to the 4.3 milestone Apr 19, 2020
remkop added a commit that referenced this issue Apr 19, 2020
* apply default value only if value is missing
* ensure options are reset to their initial value in subcommands when the `CommandLine` object is reused
@remkop
Copy link
Owner

remkop commented Apr 19, 2020

Fixed in master.

To verify this, check out picocli master locally, and build it with publishToMavenLocal. That should publish picocli-4.2.1-SNAPSHOT to your local .m2 Maven cache.

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

You can then try this in a project that uses the info.picocli:picocli:4.2.1-SNAPSHOT dependency. Feedback very welcome!

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

2 participants