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

Support for generic types in containers #1401

Merged
merged 2 commits into from
Feb 10, 2022

Conversation

mgorniew
Copy link
Contributor

@mgorniew mgorniew commented Aug 2, 2021

Improvement for issue #1396. This will also add CommandLine.UseDefaultConverter which can be used as marker for default converter fallback.

Copy link
Owner

@remkop remkop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the very long delay. I finally have some time to look at picocli again.

I am okay to merge this PR, but I would prefer not to include the UseDefaultConverter API.
Can you take a look and let me know what you think?

*
* Instances of this class will throw UnsupportedOperationException for {@link #convert(String)} method.
*/
public static final class UseDefaultConverter implements ITypeConverter<Object> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to introduce this UseDefaultConverter class as part of this PR?
Applications can use the CommandLine::registerConverter method:

new CommandLine(new MyApp())
    .registerConverter(GenericValue.class, GenericValueConverter.class)
    .execute(args);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UseDefaultConverter is only for use cases when we need to provide converters for multiple parameters and for some of them we simple want to use default, e.g.:

           @Option(names = "-D", converter = {UseDefaultConverter.class, GenericValueConverter.class})
            Map<String, GenericValue<?>> values;

In this case I would need to provided custom convert for String class also since BuiltIt.StringConverter is private.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that. Is there any issue with using the CommandLine::registerConverter method to register the GenericValueConverter?

Copy link
Contributor Author

@mgorniew mgorniew Nov 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With global registration there may be issue with type erasure. If I want to use two different converters for same GenericValue<T> (e.g. in one parameter GenericValue<Integer> and in other GenericValue<String>) then I won't be able to this with CommandLine::registerConverter

@remkop remkop linked an issue Nov 25, 2021 that may be closed by this pull request
@remkop
Copy link
Owner

remkop commented Nov 29, 2021

Okay, I see now, thanks for the clarification.

Without the UseDefaultConverter, this PR can be part of the next bugfix release (4.6.3).
Adding UseDefaultConverter is an API change, so (since picocli uses semantic versioning), this would have to go into the 4.7.0 release.

I like to add a bit more functionality before doing a 4.7.0 release, so this may take a bit more time.

Would you be open to the idea of splitting this up?
If you can take the UseDefaultConverter change out of this PR and put it into a separate PR, then that can go into the 4.7.0 release, and I can merge the remainder of this PR into master, and we can do a 4.6.3 release that includes this relatively soon.

@remkop remkop added this to the 4.6.3 milestone Dec 8, 2021
@remkop remkop modified the milestones: 4.6.3, 4.7 Jan 14, 2022
remkop
remkop previously approved these changes Feb 10, 2022
@remkop remkop merged commit 98f2647 into remkop:main Feb 10, 2022
remkop added a commit that referenced this pull request Feb 10, 2022
remkop added a commit that referenced this pull request Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use generic types in containers (e.g. List, Map)
2 participants