remkop / picocli

Picocli is a modern framework for building powerful, user-friendly, GraalVM-enabled command line apps with ease. It supports colors, autocompletion, subcommands, and more. In 1 source file so apps can include as source & avoid adding a dependency. Written in Java, usable from Groovy, Kotlin, Scala, etc.
https://picocli.info
Apache License 2.0
4.9k stars 421 forks source link

Add support for pluggable ITypeConverterFactory #1804

Open remkop opened 2 years ago

remkop commented 2 years ago

(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?

garretwilson commented 2 years ago

@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:

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 commented 2 years ago

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 commented 2 years ago

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](https://picocli.info/apidocs-all/info.picocli/picocli/CommandLine.Model.ParserSpec.html#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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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.