Selectively disable type converter registration (was: Unexpected logging message)

[pditommaso]

Migrating an app to picocli (3.5.2) I'm getting the following unexpected logging message.

Aug 24, 2018 3:01:33 PM java.util.logging.LogManager$RootLogger log
SEVERE: Failed to resolve default logging config file: config/java.util.logging.properties

That's generated while instantiating the top CommandLine instance. Is there's any reason about that? It looks quite weird because at my knowledge picocli does not use any logging library.

[remkop]

Hi, yes that is strange. As you said, picocli does not use any logging library, not even java.util.logging.

Is it possible picocli created an instance of your domain object, which (directly or indirectly) does logging in its constructor?

[pditommaso]

It turns out it produced when executing this line

 BuiltIn.registerIfAvailable(converterRegistry, tracer, "java.sql.Connection", "java.sql.DriverManager","getConnection", String.class);

I think that triggers some drivers provided by other deps. I also find all that converters a bit overkill. Is there anyway to override the default registerBuiltInConverters mechanism? at a first look, I would say no.

[remkop]

This is still very odd. All that the picocli code does is load the java.sql.Connection class and the java.sql.DriverManager class. No instances are created until needed. Out of interest, which JDBC driver or dependency produces the logging?

About making converter registration configurable: currently it isn't, but I would not object to adding some mechanism to selectively disable registration of some built-in converters. Perhaps via a system property; for example if -Dpicocli.skip.converters=java.sql.Connection,java.sql.Driver is specified, these two converters would not be registered.

[pditommaso]

Out of interest, which JDBC driver or dependency produces the logging?

Not sure about that. I think it's the caused by the Apache Ignite deps in my project.

but I would not object to adding some mechanism to selectively disable registration

If so I would propose -Dpicocli.converters.includes=xx and -Dpicocli.converters.excludes=xx

[remkop]

If so I would propose -Dpicocli.converters.includes=xx and -Dpicocli.converters.excludes=xx

What would -Dpicocli.converters.includes=xx do?

[pditommaso]

It would be applied only the classes specified

[remkop]

I can see the need for selectively excluding some of the built-in type converters (for example to suppress this strange logging you are experiencing), but I don't see how it would be useful to have a system property that excludes all converters by default and only includes some specified ones. Can you explain the use case behind this?

[remkop]

To clarify: I'm okay with the -Dpicocli.converters.excludes=x,x enhancement, and support for a -Dpicocli.converters.includes property can be added when a use case arises.

[pditommaso]

Yes, I was suggesting to have a precise control on the converter classes include. Having a excludes clause does not prevent that new classes may be added implicitly added by a future version of picocli. Instead having an explicit includes would guarantees that wont' change over time.

I know it's a bit paranoid, but IMO adding not required converts, in particular via reflection, just add an unneeded overhead startup time and may cause unexpected side effects, as the one reported.

[remkop]

The includes sounds a bit overkill to me but if you’re willing to provide a PR with unit tests I’m fine with your proposal.

It’s not obvious for users which converters are loaded via reflection and which aren’t. So, if we do this, these properties should probably be effective for all converters. Do you agree?

[pditommaso]

Thinking twice, IMO it would make more sense to allow to specify the Interpreter as a constructor argument or a class attribute. Being so the user could override the registerBuiltInConverters method to provide their own converters.

[remkop]

I’m not ready to make Interpreter public API...