picocli icon indicating copy to clipboard operation
picocli copied to clipboard

Add support for pluggable ITypeConverterFactory

Open remkop opened this issue 1 year ago • 6 comments

(See discussion in https://github.com/remkop/picocli/issues/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?

remkop avatar Sep 08 '22 10:09 remkop

@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 #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 avatar Sep 08 '22 22:09 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.

garretwilson avatar Sep 08 '22 22:09 garretwilson

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 avatar Sep 09 '22 00:09 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.

remkop avatar Sep 09 '22 00:09 remkop

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.

garretwilson avatar Sep 09 '22 01:09 garretwilson

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.

remkop avatar Dec 24 '22 08:12 remkop