Don't use a global variable

Simplify new APIs

Author: garydgregory

src/main/java/org/apache/commons/cli/Option.java

@@ -582,7 +582,7 @@ public int getArgs() {
      * @since 1.7.0
      */
     public Converter<?, ?> getConverter() {
-        return converter == null ? TypeHandler.getConverter(type) : converter;
+        return converter == null ? TypeHandler.getDefault().getConverter(type) : converter;
     }
 
     /**

src/main/java/org/apache/commons/cli/OptionBuilder.java

@@ -109,7 +109,7 @@ public static Option create(final String opt) throws IllegalArgumentException {
             option.setOptionalArg(optionalArg);
             option.setArgs(argCount);
             option.setType(type);
-            option.setConverter(TypeHandler.getConverter(type));
+            option.setConverter(TypeHandler.getDefault().getConverter(type));
             option.setValueSeparator(valueSeparator);
             option.setArgName(argName);
         } finally {

src/main/java/org/apache/commons/cli/PatternOptionBuilder.java

@@ -21,6 +21,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more
 import java.io.FileInputStream;
 import java.net.URL;
 import java.util.Date;
+import java.util.Map;
 
 /**
  * Allows Options to be created from a single String. The pattern contains various single character flags and via an
@@ -104,14 +105,10 @@ public class PatternOptionBuilder {
     public static final Class<URL> URL_VALUE = URL.class;
 
     /** The converter to use for Unimplemented data types */
-    static final Converter<?, UnsupportedOperationException> UNSUPPORTED = s -> {
+    private static final Converter<?, UnsupportedOperationException> UNSUPPORTED = s -> {
         throw new UnsupportedOperationException("Not yet implemented");
     };
 
-    static {
-        registerTypes();
-    }
-
     /**
      * Retrieve the class that {@code ch} represents.
      *
@@ -202,7 +199,10 @@ public static Options parsePattern(final String pattern) {
                 required = true;
             } else {
                 type = getValueType(ch);
-                converter = TypeHandler.getConverter(getValueType(ch));
+                final Map<Class<?>, Converter<?, ? extends Throwable>> map = TypeHandler.createDefaultMap();
+                // Backward compatibility (probably).
+                map.put(FILES_VALUE, unsupported());
+                converter = new TypeHandler(map).getConverter(getValueType(ch));
             }
         }
 
@@ -216,15 +216,6 @@ public static Options parsePattern(final String pattern) {
         return options;
     }
 
-    /**
-     * Registers custom {@code Converter}s with the {@code TypeHandler}.
-     *
-     * @since 1.7.0
-     */
-    public static void registerTypes() {
-        TypeHandler.register(PatternOptionBuilder.FILES_VALUE, unsupported());
-    }
-
     @SuppressWarnings("unchecked")
     static <T> T unsupported() {
         return (T) UNSUPPORTED;

src/main/java/org/apache/commons/cli/TypeHandler.java

@@ -38,25 +38,13 @@ Licensed to the Apache Software Foundation (ASF) under one or more
  */
 public class TypeHandler {
 
-    /** Value of hex conversion of strings */
-    private static final int HEX_RADIX = 16;
-
     /**
-     * Map of Class to Converter.
-     * <p>
-     * The Class type parameter matches the Converter's first generic type.
-     * </p>
+     * The default TypeHandler.
      */
-    private static Map<Class<?>, Converter<?, ? extends Throwable>> converterMap = createDefaultMap();
+    private static final TypeHandler DEFAULT = new TypeHandler();
 
-    /**
-     * Unregisters all Converters.
-     *
-     * @since 1.7.0
-     */
-    public static void clear() {
-        converterMap.clear();
-    }
+    /** Value of hex conversion of strings */
+    private static final int HEX_RADIX = 16;
 
     /**
      * Returns the class whose name is {@code className}.
@@ -82,7 +70,13 @@ public static Date createDate(final String string) {
         return createValueUnchecked(string, Date.class);
     }
 
-    private static Map<Class<?>, Converter<?, ? extends Throwable>> createDefaultMap() {
+    /**
+     * Creates a default converter map.
+     *
+     * @return a default converter map.
+     * @since 1.7.0
+     */
+    public static Map<Class<?>, Converter<?, ? extends Throwable>> createDefaultMap() {
         return putDefaultMap(new HashMap<>());
     }
 
@@ -162,7 +156,7 @@ public static URL createURL(final String string) throws ParseException {
      */
     public static <T> T createValue(final String string, final Class<T> clazz) throws ParseException {
         try {
-            return getConverter(clazz).apply(string);
+            return getDefault().getConverter(clazz).apply(string);
         } catch (final Throwable e) {
             throw ParseException.wrap(e);
         }
@@ -200,16 +194,13 @@ private static <T> T createValueUnchecked(final String string, final Class<T> cl
     }
 
     /**
-     * Gets the registered converter for the the Class, or {@link Converter#DEFAULT} if absent.
+     * Gets the default TypeHandler.
      *
-     * @param <T> The Class parameter type.
-     * @param clazz The Class to get the Converter for.
-     * @return the registered converter if any, {@link Converter#DEFAULT} otherwise.
+     * @return the default TypeHandler.
      * @since 1.7.0
      */
-    @SuppressWarnings("unchecked") // returned value will have type T because it is fixed by clazz
-    public static <T> Converter<T, ?> getConverter(final Class<T> clazz) {
-        return (Converter<T, ?>) converterMap.getOrDefault(clazz, Converter.DEFAULT);
+    public static TypeHandler getDefault() {
+        return DEFAULT;
     }
 
     /**
@@ -247,38 +238,41 @@ public static FileInputStream openFile(final String string) throws ParseExceptio
     }
 
     /**
-     * Registers a Converter for a Class. If {@code converter} is null registration is cleared for {@code clazz}, and no converter will be used in processing.
-     *
-     * @param <T>       The Class parameter type.
-     * @param clazz     the Class to register the Converter for.
-     * @param converter The Converter to associate with Class. May be null.
-     * @since 1.7.0
+     * Map of Class to Converter.
+     * <p>
+     * The Class type parameter matches the Converter's first generic type.
+     * </p>
      */
-    public static <T> void register(final Class<T> clazz, final Converter<T, ? extends Throwable> converter) {
-        if (converter == null) {
-            unregister(clazz);
-        } else {
-            converterMap.put(clazz, converter);
-        }
+    private final Map<Class<?>, Converter<?, ? extends Throwable>> converterMap;
+
+    /**
+     * Constructs a default initialized instance.
+     */
+    public TypeHandler() {
+        this(createDefaultMap());
     }
 
     /**
-     * Resets the registered Converters to the default state.
+     * Constructs a default initialized instance.
      *
+     * @param converterMap The converter map.
      * @since 1.7.0
      */
-    public static void resetConverters() {
-        converterMap.clear();
-        putDefaultMap(converterMap);
+    public TypeHandler(final Map<Class<?>, Converter<?, ? extends Throwable>> converterMap) {
+        this.converterMap = converterMap;
     }
 
     /**
-     * Unregisters a Converter for a Class. If {@code converter} is null registration is cleared for {@code clazz}, and no converter will be used in processing.
+     * Gets the registered converter for the the Class, or {@link Converter#DEFAULT} if absent.
      *
-     * @param clazz     the Class to unregister.
+     * @param <T>   The Class parameter type.
+     * @param clazz The Class to get the Converter for.
+     * @return the registered converter if any, {@link Converter#DEFAULT} otherwise.
      * @since 1.7.0
      */
-    public static void unregister(final Class<?> clazz) {
-        converterMap.remove(clazz);
+    @SuppressWarnings("unchecked") // returned value will have type T because it is fixed by clazz
+    public <T> Converter<T, ?> getConverter(final Class<T> clazz) {
+        return (Converter<T, ?>) converterMap.getOrDefault(clazz, Converter.DEFAULT);
     }
+
 }

src/test/java/org/apache/commons/cli/OptionTest.java

@@ -68,6 +68,9 @@ public boolean addValue(final String value) {
         }
     }
 
+    /** Always returns the same Path. */
+    private static final Converter<Path, InvalidPathException> PATH_CONVERTER = s -> Paths.get("foo");
+
     private static void checkOption(final Option option, final String opt, final String description, final String longOpt, final int numArgs,
             final String argName, final boolean required, final boolean optionalArg, final char valueSeparator, final Class<?> cls, final String deprecatedDesc,
             final Boolean deprecatedForRemoval, final String deprecatedSince) {
@@ -266,13 +269,8 @@ public void testHashCode() {
         assertNotEquals(Option.builder("test").build().hashCode(), Option.builder("test").longOpt("long test").build().hashCode());
     }
 
-    /** Always returns the same Path. */
-    private static final Converter<Path, InvalidPathException> PATH_CONVERTER = s -> Paths.get("foo");
-
     @Test
     public void testSerialization() throws IOException, ClassNotFoundException {
-        // TODO No global map!
-        TypeHandler.resetConverters();
         final Option option = Option.builder("o").type(TypeHandlerTest.Instantiable.class).build();
         assertEquals(Converter.DEFAULT, option.getConverter());
         Option roundtrip = roundTrip(option);
@@ -284,16 +282,10 @@ public void testSerialization() throws IOException, ClassNotFoundException {
         assertEquals(Converter.DEFAULT, roundtrip.getConverter());
         // verify registered class converters and verifiers do not get reset to default.
         // converters are NOT Serializable, use a serialization proxy if you want that.
-        try {
-            TypeHandler.register(Path.class, PATH_CONVERTER);
-            // verify earlier values still set.
-            assertEquals(Converter.DATE, option.getConverter());
-            roundtrip = roundTrip(option);
-            assertEquals(Converter.DEFAULT, roundtrip.getConverter());
-        } finally {
-            // TODO No global map!
-            TypeHandler.resetConverters();
-        }
+        // verify earlier values still set.
+        assertEquals(Converter.DATE, option.getConverter());
+        roundtrip = roundTrip(option);
+        assertEquals(Converter.DEFAULT, roundtrip.getConverter());
     }
 
     @Test

src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java

@@ -21,6 +21,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -33,18 +34,13 @@ Licensed to the Apache Software Foundation (ASF) under one or more
 import java.util.Date;
 import java.util.Vector;
 
-import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 
 /**
  * Test case for the PatternOptionBuilder class.
  */
 @SuppressWarnings("deprecation") // tests some deprecated classes
 public class PatternOptionBuilderTest {
-    @BeforeAll
-    public static void setup() {
-        PatternOptionBuilder.registerTypes();
-    }
 
     @Test
     public void testClassPattern() throws Exception {
@@ -164,12 +160,7 @@ public void testSimplePattern() throws Exception {
         assertEquals(new URL("https://commons.apache.org"), line.getOptionObject('t'), "url flag t");
 
         // FILES NOT SUPPORTED YET
-        try {
-            assertEquals(new File[0], line.getOptionObject('m'), "files flag m");
-            fail("Multiple files are not supported yet, should have failed");
-        } catch (final UnsupportedOperationException uoe) {
-            // expected
-        }
+        assertThrows(UnsupportedOperationException.class, () -> line.getOptionObject('m'));
 
         assertEquals(expectedDate, line.getOptionObject('z'), "date flag z");