garydgregory commented on code in PR #216: URL: https://github.com/apache/commons-cli/pull/216#discussion_r1442823252
########## src/main/java/org/apache/commons/cli/converters/Converter.java: ########## @@ -0,0 +1,74 @@ +/* + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ +package org.apache.commons.cli.converters; + +import java.io.File; +import java.net.URL; +import java.text.SimpleDateFormat; +import java.util.Date; + +/** + * The definition of the functional interface to call when doing a conversion. + * Like {@code Function<String,T>} but can throw an Exception. + * + * @param <T> The return type for the function. + * @since 1.7 + */ +@FunctionalInterface +public interface Converter<T> { Review Comment: Probably should extend `Function<String, T>`. ########## src/main/java/org/apache/commons/cli/TypeHandler.java: ########## @@ -19,190 +19,306 @@ Licensed to the Apache Software Foundation (ASF) under one or more import java.io.File; import java.io.FileInputStream; -import java.io.FileNotFoundException; -import java.net.MalformedURLException; +import java.math.BigDecimal; +import java.math.BigInteger; import java.net.URL; import java.util.Date; +import java.util.HashMap; +import java.util.Map; + +import org.apache.commons.cli.converters.Converter; +import org.apache.commons.cli.converters.Verifier; /** - * This is a temporary implementation. TypeHandler will handle the pluggableness of OptionTypes and it will direct all - * of these types of conversion functionalities to ConvertUtils component in Commons already. BeanUtils I think. + * TypeHandler will handle the pluggable conversion and verification of + * Option types. It handles the mapping of classes to bot converters and verifiers. + * It provides the default conversion and verification methods when converters and verifiers + * are not explicitly set. + * <p> + * If Options are serialized and deserialized their converters and verifiers will revert to the + * defaults defined in this class. To correctly de-serialize Options with custom converters and/or + * verifiers, using the default serialization methods, this class should be properly configured with the custom + * converters and verifiers for the specific class. + * </p> */ public class TypeHandler { + + /** Value of hex conversion of strings */ + private static final int HEX_RADIX = 16; + + /** Map of classes to converters. */ + private static Map<Class<?>, Converter<?>> converterMap = new HashMap<>(); + + /** Map of classes to verifiers. */ + private static Map<Class<?>, Verifier> verifierMap = new HashMap<>(); + + static { + resetConverters(); + resetVerifiers(); + } + + /** + * Resets the registered Converters to the default state. + * @since 1.7 + */ + public static void resetConverters() { + converterMap.clear(); + converterMap.put(Object.class, Converter.OBJECT); + converterMap.put(Class.class, Converter.CLASS); + converterMap.put(Date.class, Converter.DATE); + converterMap.put(File.class, Converter.FILE); + converterMap.put(Number.class, Converter.NUMBER); + converterMap.put(URL.class, Converter.URL); + converterMap.put(FileInputStream.class, s -> new FileInputStream(s)); + converterMap.put(Long.class, Long::parseLong); + converterMap.put(Integer.class, Integer::parseInt); + converterMap.put(Short.class, Short::parseShort); + converterMap.put(Byte.class, Byte::parseByte); + converterMap.put(Character.class, s -> { + if (s.startsWith("\\u")) { + return Character.toChars(Integer.parseInt(s.substring(2), HEX_RADIX))[0]; + } else { + return s.charAt(0); + } }); + converterMap.put(Double.class, Double::parseDouble); + converterMap.put(Float.class, Float::parseFloat); + converterMap.put(BigInteger.class, s -> new BigInteger(s)); + converterMap.put(BigDecimal.class, s -> new BigDecimal(s)); + } + + /** + * Unregisters all Converters. + * @since 1.7 + */ + public static void noConverters() { + converterMap.clear(); + } + + /** + * Resets the registered Verifiers to the default state. + * @since 1.7 + */ + public static void resetVerifiers() { + verifierMap.clear(); + verifierMap.put(Object.class, Verifier.CLASS); + verifierMap.put(Class.class, Verifier.CLASS); + verifierMap.put(Number.class, Verifier.NUMBER); + + verifierMap.put(Long.class, Verifier.INTEGER); + verifierMap.put(Integer.class, Verifier.INTEGER); + verifierMap.put(Short.class, Verifier.INTEGER); + verifierMap.put(Byte.class, Verifier.INTEGER); + + verifierMap.put(Double.class, Verifier.NUMBER); + verifierMap.put(Float.class, Verifier.NUMBER); + verifierMap.put(BigInteger.class, Verifier.INTEGER); + verifierMap.put(BigDecimal.class, Verifier.NUMBER); + } + + /** + * Unregisters all Verifiers. + * @since 1.7 + */ + public static void noVerifiers() { + verifierMap.clear(); + } + + /** + * Registers a Converter and Verifier for a Class. If @code converter} or + * {@code verifier} are null their respective registrations are cleared for {@code clazz}, and + * defaults will be used in processing. + * + * @param clazz the Class to register the Converter and Verifier to. + * @param converter The Converter to associate with Class. May be null. + * @param verifier The Verifier to associate with Class. May be null. + * @since 1.7 + */ + public static void register(Class<?> clazz, Converter<?> converter, Verifier verifier) { + if (converter == null) { + converterMap.remove(clazz); + } else { + converterMap.put(clazz, converter); + } + + if (verifier == null) { + verifierMap.remove(clazz); + } else { + verifierMap.put(clazz, verifier); + } + } + + /** + * Gets the converter for the the Class. Never null. + * @param clazz The Class to get the Converter for. + * @return the registered converter if any, {@link Converter#DEFAULT} otherwise. + * @since 1.7 + */ + public static Converter<?> getConverter(Class<?> clazz) { + Converter<?> converter = converterMap.get(clazz); + return converter == null ? Converter.DEFAULT : converter; + } + + /** + * Gets the verifier for the Class. Never null. + * @param clazz the Class to get the Verifier for. + * @return the registered verifier if any, {@link Verifier#DEFAULT} otherwise. + * @since 1.7 + */ + public static Verifier getVerifier(Class<?> clazz) { + Verifier verifier = verifierMap.get(clazz); + return verifier == null ? Verifier.DEFAULT : verifier; + } + /** * Returns the class whose name is {@code className}. * - * @param className the class name - * @return The class if it is found - * @throws ParseException if the class could not be found + * @param className the class name + * @return The class if it is found + * @throws ParseException if the class could not be found + * @deprecated use {@link #createValue(String, Class)} */ + @Deprecated // since 1.7 public static Class<?> createClass(final String className) throws ParseException { - try { - return Class.forName(className); - } catch (final ClassNotFoundException e) { - throw new ParseException("Unable to find the class: " + className); - } + return createValue(className, Class.class); } /** - * Returns the date represented by {@code str}. - * <p> - * This method is not yet implemented and always throws an {@link UnsupportedOperationException}. + * Returns the date represented by {@code str}. <p> This method is not yet + * implemented and always throws an {@link UnsupportedOperationException}. * - * @param str the date string - * @return The date if {@code str} is a valid date string, otherwise return null. - * @throws UnsupportedOperationException always + * @param str the date string + * @return The date if {@code str} is a valid date string, otherwise + * return null. + * @deprecated use {@link #createValue(String, Class)} */ + @Deprecated // since 1.7 public static Date createDate(final String str) { - throw new UnsupportedOperationException("Not yet implemented"); + try { + return createValue(str, Date.class); + } catch (ParseException e) { + throw new RuntimeException(e); + } } /** * Returns the File represented by {@code str}. * - * @param str the File location - * @return The file represented by {@code str}. + * @param str the File location Review Comment: Please revert the formatting changes. It makes the PR noisier, larger and takes longer to review. ########## src/test/java/org/apache/commons/cli/OptionTest.java: ########## @@ -75,70 +85,69 @@ private static void checkOption(final Option option, final String opt, final Str @Test public void testBuilderInsufficientParams1() { - assertThrows(IllegalArgumentException.class, () -> - Option.builder().desc("desc").build()); + assertThrows(IllegalArgumentException.class, () -> Option.builder().desc("desc").build()); } @Test public void testBuilderInsufficientParams2() { - assertThrows(IllegalArgumentException.class, () -> - Option.builder(null).desc("desc").build()); + assertThrows(IllegalArgumentException.class, () -> Option.builder(null).desc("desc").build()); } @Test public void testBuilderInvalidOptionName1() { - assertThrows(IllegalArgumentException.class, () -> - Option.builder().option("invalid?")); + assertThrows(IllegalArgumentException.class, () -> Option.builder().option("invalid?")); } @Test public void testBuilderInvalidOptionName2() { - assertThrows(IllegalArgumentException.class, () -> - Option.builder().option("invalid@")); + assertThrows(IllegalArgumentException.class, () -> Option.builder().option("invalid@")); } @Test public void testBuilderInvalidOptionName3() { - assertThrows(IllegalArgumentException.class, () -> - Option.builder("invalid?")); + assertThrows(IllegalArgumentException.class, () -> Option.builder("invalid?")); } @Test public void testBuilderInvalidOptionName4() { - assertThrows(IllegalArgumentException.class, () -> - Option.builder("invalid@")); + assertThrows(IllegalArgumentException.class, () -> Option.builder("invalid@")); } @Test public void testBuilderMethods() { final char defaultSeparator = (char) 0; - checkOption(Option.builder("a").desc("desc").build(), "a", "desc", null, Option.UNINITIALIZED, null, false, false, defaultSeparator, String.class); - checkOption(Option.builder("a").desc("desc").build(), "a", "desc", null, Option.UNINITIALIZED, null, false, false, defaultSeparator, String.class); - checkOption(Option.builder("a").desc("desc").longOpt("aaa").build(), "a", "desc", "aaa", Option.UNINITIALIZED, null, false, false, defaultSeparator, - String.class); - checkOption(Option.builder("a").desc("desc").hasArg(true).build(), "a", "desc", null, 1, null, false, false, defaultSeparator, String.class); - checkOption(Option.builder("a").desc("desc").hasArg(false).build(), "a", "desc", null, Option.UNINITIALIZED, null, false, false, defaultSeparator, - String.class); - checkOption(Option.builder("a").desc("desc").hasArg(true).build(), "a", "desc", null, 1, null, false, false, defaultSeparator, String.class); - checkOption(Option.builder("a").desc("desc").numberOfArgs(3).build(), "a", "desc", null, 3, null, false, false, defaultSeparator, String.class); - checkOption(Option.builder("a").desc("desc").required(true).build(), "a", "desc", null, Option.UNINITIALIZED, null, true, false, defaultSeparator, - String.class); - checkOption(Option.builder("a").desc("desc").required(false).build(), "a", "desc", null, Option.UNINITIALIZED, null, false, false, defaultSeparator, - String.class); - - checkOption(Option.builder("a").desc("desc").argName("arg1").build(), "a", "desc", null, Option.UNINITIALIZED, "arg1", false, false, defaultSeparator, - String.class); - checkOption(Option.builder("a").desc("desc").optionalArg(false).build(), "a", "desc", null, Option.UNINITIALIZED, null, false, false, defaultSeparator, - String.class); - checkOption(Option.builder("a").desc("desc").optionalArg(true).build(), "a", "desc", null, 1, null, false, true, defaultSeparator, - String.class); - checkOption(Option.builder("a").desc("desc").valueSeparator(':').build(), "a", "desc", null, Option.UNINITIALIZED, null, false, false, ':', - String.class); - checkOption(Option.builder("a").desc("desc").type(Integer.class).build(), "a", "desc", null, Option.UNINITIALIZED, null, false, false, defaultSeparator, - Integer.class); - checkOption(Option.builder().option("a").desc("desc").type(Integer.class).build(), "a", "desc", null, Option.UNINITIALIZED, null, false, false, - defaultSeparator, Integer.class); + checkOption(Option.builder("a").desc("desc").build(), "a", "desc", null, Option.UNINITIALIZED, null, false, Review Comment: What's changed here? If it's just formatting, please revert, I don't want to take the time to tease apart formatting from feature changes. ########## src/main/java/org/apache/commons/cli/converters/Verifier.java: ########## @@ -0,0 +1,74 @@ +/* + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ +package org.apache.commons.cli.converters; + +import java.util.regex.Pattern; + +/** + * The definition of the functional interface to call when verifying a string + * for input Like {@code Predicate<String>} but can throw a RuntimeException. + * @since 1.7 + */ +@FunctionalInterface +public interface Verifier { Review Comment: Probably more flexible if this extends `Predicate<String>`. ########## src/main/java/org/apache/commons/cli/converters/Verifier.java: ########## @@ -0,0 +1,74 @@ +/* + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ +package org.apache.commons.cli.converters; + +import java.util.regex.Pattern; + +/** + * The definition of the functional interface to call when verifying a string + * for input Like {@code Predicate<String>} but can throw a RuntimeException. + * @since 1.7 Review Comment: The next feature version will be `1.7.0` (major.minor.maintenance). ########## src/main/java/org/apache/commons/cli/CommandLine.java: ########## @@ -364,29 +365,79 @@ public Object getParsedOptionValue(final char opt) throws ParseException { * @throws ParseException if there are problems turning the option value into the desired type * @see PatternOptionBuilder * @since 1.5.0 + * @param <T> The return type for the method. */ - public Object getParsedOptionValue(final Option option) throws ParseException { + public <T> T getParsedOptionValue(final Option option) throws ParseException { + return getParsedOptionValue(option, null); + } + + /** + * Gets a version of this {@code Option} converted to a particular type. + * + * @param opt the name of the option. + * @return the value parsed into a particular object. + * @throws ParseException if there are problems turning the option value into the desired type + * @see PatternOptionBuilder + * @since 1.2 Review Comment: The next feature version will be `1.7.0` (major.minor.maintenance). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
