Add support for pluggable ITypeConverterFactory

[remkop]

(See discussion in #1707)

There are cases where applications want programmatic control of the type converter used.
The use case mentioned in #1707 is a single type converter for many Enum classes.
Another potential use case is a single type converter for all implementations of an interface.

The proposed interface is as follows:

/**
 * Interface that allows customization of what type converters to use for specific types
 * and options or positional parameters.
 * If a type converter factory is set for a command, then this factory is first queried to produce a type converter.
 * If no type converter factory is set, or the factory returns a null type converter, 
 * then the default picocli logic is used to attempt to find a type converter.
 * @since 4.x
 */
interface ITypeConverterFactory {
    /**
     * Returns a type converter for the specified type for the specified option or positional parameter,
     * or returns {@code null} if the default converter should be used.
     * 
     * @param type the type (or supertype) for which to return a type converter
     * @param argSpec the option or positional parameter for which to return a type converter
     * @return a type converter or {@code null}
     * @since 4.x
     */
    ITypeConverter<? extends T> createTypeConverter(Class<T> type, ArgSpec argSpec);
}

As is the convention with other getters/setters, applications will be able to install a custom type converter factory for the full command hierarchy via a getter and setter in CommandLine:

/** Sets the specified type converter factory for this command and all subcommands. */
CommandLine::setTypeConverterFactory(ITypeConverterFactory) : void

/** Returns the custom type converter factory for this command, or {@code null} if none was set. */
CommandLine::getTypeConverterFactory() : ITypeConverterFactory

As is the convention with other getters/setters, applications will be able to install a custom type converter factory on a per-command basis via a getter and setter in CommandSpec:

/** Sets the specified type converter factory for this command only. */
CommandSpec::typeConverterFactory(ITypeConverterFactory) : void

/** Returns the custom type converter factory for this command, or {@code null} if none was set. */
CommandSpec::typeConverterFactory() : ITypeConverterFactory

There is no default implementation, that is, by default, CommandLine::getTypeConverterFactory() returns null.
If a type converter factory is set, and it returns a non-null type converter, then this converter is used, otherwise the default picocli logic is used to attempt to find a type converter.

@garretwilson Thoughts/feedback?

[garretwilson]

@remkop I think that provides the functionality needed to hook into type conversion for all enums types.

One potential problem is that it isn't very flexible. That is if someone else installs their own type converter factory, even just to handle a single type that I'm not even interested in, then it would completely blow away my custom type converter factory. One workaround is that a library could need to find the current type converter factory (if any), and then if the new type converter factory didn't handle the type, it could delegate to whatever was installed before. Of course this relies on everybody following this convention, and it would break if someone tries to uninstall their type converter factory and re-install the previous one (which would blow away any later registrations).

Obviously we don't expect to have many libraries dipping in and registering type converter factories independently. However this hypothetical scenario I think illustrates that this approach has some drawbacks.

Instead of having a single get/set type converter variable, then, it might be better to keep an internal list. Users would use addTypeConverterFactory() and removeTypeConverterFatory() (or "register"/"unregister); this would simply add the type converter to a list (or a linked hash set to prevent duplicates). Then picocli could simply iterate this list backwards and choose the first non-null response from the registered type converter factories. This approach has several benefits:

• It plays just fine with multiple sources/libraries adding type converter factories; they don't "trample" each other. Simply the last one registered takes precedence.
• You can now have different type converter factories for different types. I can have one for enums, and another for the FooBar interface. I don't have to stick all the logic in one type converter factory (which I wouldn't want to do anyway).
• And this way you can actually extract the current "built-in" type conversion logic (I mentioned this in Convert enum snake_case to and from kebab-case. #1707) into an ITypeConverterFactory that sits at position 0 in the list—it always has the last fall-back priority, and it can never be removed. Suddenly your existing code is more modularized, more consolidated, and more elegant.

Lastly I haven't looked into what's in the ArgSpec, but if I don't need it I'm sure it doesn't hurt to pass it.

[garretwilson]

One other potential confusion (not to deter you; just to mention it) is that you'll still have the per-type registration system, and if someone registers an overriding ITypeConverterFactory it could potentially "mask" the per-type registration. It would seem obvious once someone realizes it, but you might mention it in the docs to prevent potential confusion.

[remkop]

Lastly I haven't looked into what's in the ArgSpec, but if I don't need it I'm sure it doesn't hurt to pass it.

From the ArgSpec, one can get the CommandSpec, which in turn gives access to everything, including the ParserSpec; this is where parser configuration settings are held, which includes things like caseInsensitiveEnumValuesAllowed, which may be relevant/necessary for type conversion implementations.

Anothere benefit of passing the ArgSpec is that it allows the factory to produce different type converters for different options/positional params, regardless of the type they are associated with.

[remkop]

I am not opposed to the idea of having a list of type conversion factories, with add and remove methods instead of get/set methods, iterating backwards, and having the existing logic at position 0 as the last fall-back factory.

Overall, I am having some second thoughts about this whole endeavor in the sense that it feels like a lot of infrastructure for a relatively small benefit.

[garretwilson]

Overall, I am having some second thoughts about this whole endeavor in the sense that it feels like a lot of infrastructure for a relatively small benefit.

Whatever you decide, just tell me what I need to do in my base application to convert the case of enums automatically. Currently the case of enums do not match the convention used by other parts of the application. Currently it seems to me impossible to change. I don't feel that copying and pasting essentially your entire source code just to change a line or two to effect this functionality is reasonable.

Sheesh if getEnumTypeConverter(final Class<?> type) was marked protected then I could at least subclass CommandLine (that's the class it's in, right?). But right now it's basically impossible for this functionality.

And the fact that getEnumTypeConverter(final Class<?> type) even exists is code smell indicating that there was an issue requiring a "special case" for enums in the first place.

But just tell me how to work around this. I only need to do it once. You don't have to create a new ITypeConverterFactory infrastructure if you don't want to. But we need something! Right now we've got nothing.

[remkop]

I will probably make getEnumTypeConverter(final Class<?> type) protected in 4.8.

And I don't agree that the existence of that method is a code smell. Enums are special because no converter needs to be registered for them, whereas all other types need type-specific converters.

[garretwilson]

Hi @remkop . Dropping by a couple of years later to make sure this doesn't get forgotten. We went through a lot of discussion, but it all comes down to my earlier comment:

Whatever you decide, just tell me what I need to do in my base application to convert the case of enums automatically. Currently the case of enums do not match the convention used by other parts of the application. Currently it seems to me impossible to change. I don't feel that copying and pasting essentially your entire source code just to change a line or two to effect this functionality is reasonable.

That's it. I need to have a way to plug in a converter that knows the type of enum I'm converting to. The existing design knows that's a need, because getEnumTypeConverter(final Class<?> type) (unlike the other converters) provides the type. That's what's missing here. See in particular my comment on another ticket:

… I see that you have this:

private ITypeConverter<Object> getEnumTypeConverter(final Class<?> type)

Unfortunately there's no way for me to override that, either. 😢

And I note that you actually call this method explicitly if there is no registered converter, passing the type!

if (type.isEnum()) {
  return getEnumTypeConverter(type);
}

So you must have already realized when you were writing the code that there was a problem with the conversion interface model (i.e. not providing a type to the converter at conversion time).

If if enums are the only thing that need to know the type at conversion time (I doubt it), I still need way to patch in my custom conversion to override the dynamic creation you do inside getEnumTypeConverter().

Let me put it another way: from the perspective of API design, if it was valid for the library implementor to need to call getEnumTypeConverter(final Class<?> type) and create the enum converter separately, it's not unreasonable that a user might want the same functionality to effect the same conversion but in a different manner.