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

Create empty Map for Map @Option in command methods #533

Closed
bobtiernay-okta opened this issue Oct 28, 2018 · 13 comments
Closed

Create empty Map for Map @Option in command methods #533

bobtiernay-okta opened this issue Oct 28, 2018 · 13 comments

Comments

@bobtiernay-okta
Copy link
Contributor

bobtiernay-okta commented Oct 28, 2018

Currently the following will result in a null Map being passed to the method:

public Status exec(@Option(names = "-e", description = "Environmental variables in key=value format") Map<String, String> vars) {
// ...
}

From the docs:

If the field is null, picocli will instantiate it when the option or positional parameter is matched. If the type is not a concrete class, picocli will instantiate a LinkedHashMap to preserve the input ordering.

However this is isn't a field so the expectation is unclear. Ideally this would be initialized to an empty Map. Updating the documentation to account for command methods would also be useful.

@remkop
Copy link
Owner

remkop commented Oct 28, 2018

The convention in picocli is to not modify annotated fields (or parameters) that weren’t matched on the command line, unless a default value was specified.

For object fields this means that if they’re declared without an initial value, they stay null. For parameters the behavior is similar.

Do you feel like providing a pull request to improve the documentation? (No need to change the HTML file, it is generated from the index.adoc file.)

@remkop
Copy link
Owner

remkop commented Oct 31, 2018

@bobtiernay-okta Does this answer your question?

@bobtiernay-okta
Copy link
Contributor Author

Yes it does thanks. Sorry, I planned to say I would help add a note to the docs. Might be able to get to it later in the week. Thanks!

@remkop
Copy link
Owner

remkop commented Nov 1, 2018

That would be great, thanks!

@bobtiernay-okta
Copy link
Contributor Author

So I'm trying to update the documentation but I'm struggling to square the existing docs:

If the field is null, picocli will instantiate it when the option or positional parameter is matched

with your comment above:

The convention in picocli is to not modify annotated fields (or parameters) that weren’t matched on the command line, unless a default value was specified.

In the case of an @Option that is on a command method parameter without a defaultValue specified, should't the behavior be to create it per the first sentence above?

@remkop
Copy link
Owner

remkop commented Nov 2, 2018

I created a test to demonstrate the current behaviour and actually found a bug. :-) The test is in the issue I created: #538

The test shows the expected behaviour:

  • if no options are matched, method parameters without a default will have a null value, while method parameters with a default will have the default value
  • if options are matched, the matched value is passed to the method

In the case of fields, it's similar, but slightly more complex because fields can have an initial value in their declaration, for example:

@Option(names = "-x")
Integer x = 123;
  • if the option is not matched, annotated fields without a default will be set to their initial value, and annotated fields with a default will be set to the default value. Instead of "are not modified" it's more accurate to say "are reset to the initial value".
  • if the option is matched, annotated fields are set to the matched value.

Also, when the option is matched, for Collection fields picocli will instantiate the Collection unless the field already has a non-null value (similar for Maps):

@Option(names = "-y")
List<String> y = null; // picocli creates an ArrayList instance if -y is matched

@Option(names = "-z")
List<String> z = new ArrayList<>(); // picocli adds to this existing instance if -z is matched

@remkop
Copy link
Owner

remkop commented Nov 9, 2018

Did my last comment answer your question?

@remkop
Copy link
Owner

remkop commented Nov 10, 2018

Early warning: I’m planning to do a release in a week or so. If you can provide any doc improvements this week they will be included in the upcoming release. Of course there will be other releases if that’s too soon.

@bobtiernay-okta
Copy link
Contributor Author

Will try to get to this tomorrow or Monday.

@remkop
Copy link
Owner

remkop commented Nov 11, 2018

Great, thanks!

@remkop
Copy link
Owner

remkop commented Nov 11, 2018

Hi @bobtiernay-okta, I saw the PR on #545, thank you! Is this ready to be merged and are you happy to close this ticket when #545 is merged or are there more changes in the pipeline?

@bobtiernay-okta
Copy link
Contributor Author

I think its good enough for now. I was going to add more detail but it really didn't fit in that section. I'll try to contribute a more substantial piece next time!

@remkop
Copy link
Owner

remkop commented Nov 12, 2018

Understood. I merged to PR. Do you want to keep this ticket open?

@remkop remkop closed this as completed in ef3e123 Nov 12, 2018
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