simplify Settings API
authorHans <firedrake93@gmail.com>
Fri, 26 Oct 2018 21:11:42 +0000 (16:11 -0500)
committerHans <firedrake93@gmail.com>
Fri, 26 Oct 2018 21:11:42 +0000 (16:11 -0500)
api/src/main/java/org/apache/any23/configuration/Setting.java
api/src/main/java/org/apache/any23/configuration/Settings.java
api/src/test/java/org/apache/any23/configuration/SettingsTest.java
cli/src/main/java/org/apache/any23/cli/Rover.java
core/src/main/java/org/apache/any23/writer/WriterSettings.java

index 92a3632..25f07f8 100644 (file)
@@ -22,243 +22,122 @@ import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
 import java.lang.reflect.TypeVariable;
 import java.util.HashMap;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.regex.Pattern;
 
 /**
- * Represents a {@link Setting.Key Key} paired with a compatible value.
+ * Represents a setting key paired with a compatible value.
  *
  * @author Hans Brende (hansbrende@apache.org)
  */
-public final class Setting<V> {
+public abstract class Setting<V> implements Cloneable {
+
+    private final Key<V> key;
+    private V value;
 
     /**
-     * Convenience method for creating a new setting key with the specified identifier and value class.
-     * If the desired value type is a {@link ParameterizedType} such as {@code List<String>},
-     * or custom value-checking is required, then this method is not appropriate; instead,
-     * extend the {@link Key} class directly.
-     *
-     * @param identifier a unique identifier for this key
-     * @param valueType the type of value allowed by this key
-     * @return a new {@link Key} instance initialized with the specified identifier and value type
-     * @throws IllegalArgumentException if the identifier or value type is invalid
+     * Constructs a new setting with the specified identifier and default value. This constructor must be called
+     * with concrete type arguments.
+     * @param identifier the identifier for this setting
+     * @param defaultValue the default value for this setting
+     * @throws IllegalArgumentException if the identifier or any of the type arguments were invalid
      */
-    public static <V> Key<V> newKey(String identifier, Class<V> valueType) {
-        return new Key<V>(identifier, valueType) {};
+    protected Setting(String identifier, V defaultValue) {
+        checkIdentifier(identifier);
+        this.key = new Key<>(identifier, lookupValueType(getClass(), identifier), defaultValue);
+        this.value = defaultValue;
     }
 
     /**
-     * Represents the key for a {@link Setting}.
+     * @return the identifier for this setting
      */
-    public static abstract class Key<V> {
-        private final String identifier;
-        private final Type valueType;
-
-        private Key(String identifier, Class<V> valueType) {
-            this.identifier = checkIdentifier(identifier);
-            if ((this.valueType = valueType) == null) {
-                throw new IllegalArgumentException("value type cannot be null");
-            }
-
-            if (valueType.isArray()) {
-                throw new IllegalArgumentException(identifier + " value class must be immutable");
-            } else if (valueType.getTypeParameters().length != 0) {
-                throw new IllegalArgumentException(identifier + " setting key must fill in type parameters for " + valueType.toGenericString());
-            } else if (valueType.isPrimitive()) {
-                //ensure using primitive wrapper classes
-                //so that Class.isInstance(), etc. will work as expected
-                throw new IllegalArgumentException(identifier + " value class cannot be primitive");
-            }
-        }
-
-        private static final Pattern identifierPattern = Pattern.compile("[a-z][0-9a-z]*(\\.[a-z][0-9a-z]*)*");
-        private static String checkIdentifier(String identifier) {
-            if (identifier == null) {
-                throw new IllegalArgumentException("identifier cannot be null");
-            }
-            if (!identifierPattern.matcher(identifier).matches()) {
-                throw new IllegalArgumentException("identifier does not match " + identifierPattern.pattern());
-            }
-            return identifier;
-        }
-
-        /**
-         * Constructs a new key with the specified identifier.
-         * @param identifier the identifier for this key
-         * @throws IllegalArgumentException if the identifier is invalid, or the value type was determined to be invalid
-         */
-        protected Key(String identifier) {
-            this.identifier = checkIdentifier(identifier);
-
-            Type type = valueType = getValueType();
-
-            if (type instanceof Class) {
-                if (((Class) type).isArray()) {
-                    throw new IllegalArgumentException(identifier + " value class must be immutable");
-                } else if (((Class) type).getTypeParameters().length != 0) {
-                    throw new IllegalArgumentException(identifier + " setting key must fill in type parameters for " + ((Class) type).toGenericString());
-                }
-            } else if (type instanceof GenericArrayType) {
-                throw new IllegalArgumentException(identifier + " value class must be immutable");
-            } else if (type instanceof TypeVariable) {
-                throw new IllegalArgumentException("Invalid setting key type 'Key<" + type.getTypeName() + ">' for identifier " + identifier);
-            } else if (!(type instanceof ParameterizedType)) {
-                throw new IllegalArgumentException(identifier + " invalid key type " + type + " (" + type.getClass().getName() + ")");
-            }
-        }
-
-        private Type getValueType() {
-            HashMap<TypeVariable<?>, Type> mapping = new HashMap<>();
-            Class<?> rawType = getClass();
-            assert rawType != Key.class;
-            for (;;) {
-                Type superclass = rawType.getGenericSuperclass();
-                if (superclass instanceof ParameterizedType) {
-                    rawType = (Class)((ParameterizedType) superclass).getRawType();
-                    Type[] args = ((ParameterizedType) superclass).getActualTypeArguments();
-                    if (Key.class.equals(rawType)) {
-                        Type t = args[0];
-                        return mapping.getOrDefault(t, t);
-                    }
-                    TypeVariable<?>[] vars = rawType.getTypeParameters();
-                    for (int i = 0, len = vars.length; i < len; i++) {
-                        Type t = args[i];
-                        mapping.put(vars[i], t instanceof TypeVariable ? mapping.get(t) : t);
-                    }
-                } else {
-                    rawType = (Class<?>)superclass;
-                    if (Key.class.equals(rawType)) {
-                        throw new IllegalArgumentException(getClass() + " does not supply type arguments");
-                    }
-                }
-            }
-        }
-
-        /**
-         * Subclasses may override this method to check that new settings for this key are valid.
-         * The default implementation of this method throws a {@link NullPointerException} if the new value is null and the initial value was non-null.
-         *
-         * @param initial the setting containing the initial value for this key, or null if the setting has not yet been initialized
-         * @param newValue the new value for this setting
-         * @throws Exception if the new value for this setting was invalid
-         */
-        protected void checkValue(Setting<V> initial, V newValue) throws Exception {
-            if (newValue == null && initial != null && initial.value != null) {
-                throw new NullPointerException();
-            }
-        }
-
-        private Setting<V> checked(Setting<V> origin, V value) {
-            try {
-                checkValue(origin, value);
-            } catch (Exception e) {
-                throw new IllegalArgumentException("invalid value for key '" + identifier + "': " + value, e);
-            }
-            return new Setting<>(this, value);
-        }
-
-        /**
-         * @return a new {@link Setting} object with this key and the supplied value.
-         *
-         * @throws IllegalArgumentException if the new value was invalid, as determined by:
-         * <pre>
-         *      {@code this.checkValue(null, value)}
-         * </pre>
-         *
-         * @see #checkValue(Setting, Object)
-         */
-        public final Setting<V> withValue(V value) {
-            return checked(null, value);
-        }
-
-        /**
-         * @param o the object to check for equality
-         * @return {@code this == o}
-         */
-        public final boolean equals(Object o) {
-            return super.equals(o);
-        }
-
-        /**
-         * @return the identity-based hashcode of this key
-         */
-        public final int hashCode() {
-            return super.hashCode();
-        }
-
-        public String toString() {
-            return identifier + ": " + valueType.getTypeName();
-        }
-    }
-
-    private final Key<V> key;
-    private final V value;
-
-    private Setting(Key<V> key, V value) {
-        this.key = key;
-        this.value = value;
+    public final String getIdentifier() {
+        return key.identifier;
     }
 
     /**
-     * @return the identifier for this setting
+     * Subclasses may override this method to check that new values for this setting are valid.
+     * The default implementation of this method throws a {@link NullPointerException} if the new value is null and the default value is non-null.
+     *
+     * @param newValue the new value for this setting
+     * @throws Exception if the new value for this setting is invalid
      */
-    public String getIdentifier() {
-        return key.identifier;
+    protected void checkValue(V newValue) throws Exception {
+        if (newValue == null && key.defaultValue != null) {
+            throw new NullPointerException();
+        }
     }
 
     /**
      * @return the value for this setting
      */
-    public V getValue() {
+    public final V getValue() {
         return value;
     }
 
     /**
+     * @return the default value for this setting
+     */
+    public final V getDefaultValue() {
+        return key.defaultValue;
+    }
+
+    /**
      * @return the type of value supported for this setting
      */
-    public Type getValueType() {
+    public final Type getValueType() {
         return key.valueType;
     }
 
     /**
-     * @return the supplied setting, if it has the same key as this setting
+     * @return this setting, if this setting has the same key as the supplied setting
      */
     @SuppressWarnings("unchecked")
-    public final Optional<Setting<V>> cast(Setting<?> setting) {
-        return setting == null || setting.key != this.key ? Optional.empty() : Optional.of((Setting<V>)setting);
+    public final <S extends Setting<?>> Optional<S> as(S setting) {
+        return key == ((Setting<?>)setting).key ? Optional.of((S)this) : Optional.empty();
     }
 
     /**
-     * @return a new {@link Setting} object with this setting's {@link Key Key} and the supplied value.
+     * @return a new {@link Setting} object with this setting's key and the supplied value.
      *
      * @throws IllegalArgumentException if the new value was invalid, as determined by:
      * <pre>
-     *     {@code this.key.checkValue(this, newValue)}
+     *     {@code this.checkValue(newValue)}
      * </pre>
      *
-     * @see Key#checkValue(Setting, Object)
+     * @see Setting#checkValue(Object)
      */
-    public Setting<V> withValue(V newValue) {
-        return key.checked(this, newValue);
+    public final Setting<V> withValue(V newValue) {
+        return clone(this, newValue);
+    }
+
+    @Override
+    protected final Object clone() {
+        try {
+            //ensure no subclasses override this incorrectly
+            return super.clone();
+        } catch (CloneNotSupportedException e) {
+            throw new AssertionError(e);
+        }
     }
 
     /**
-     * @return true if the supplied object is an instance of {@link Setting} and has the same key and value as this object.
+     * @return true if the supplied object is an instance of {@link Setting}
+     * and has the same key and value as this setting.
      */
     @Override
-    public boolean equals(Object o) {
+    public final boolean equals(Object o) {
         if (this == o) return true;
         if (!(o instanceof Setting)) return false;
 
         Setting<?> setting = (Setting<?>) o;
-
-        if (key != setting.key) return false;
-        return value != null ? value.equals(setting.value) : setting.value == null;
+        return key == setting.key && Objects.equals(value, setting.value);
     }
 
     @Override
-    public int hashCode() {
-        return 31 * key.hashCode() + (value != null ? value.hashCode() : 0);
+    public final int hashCode() {
+        return key.hashCode() ^ Objects.hashCode(value);
     }
 
     @Override
@@ -266,4 +145,91 @@ public final class Setting<V> {
         return key.identifier + "=" + value;
     }
 
+
+
+    ///////////////////////////////////////
+    // Private static helpers
+    ///////////////////////////////////////
+
+    private static final class Key<V> {
+        final String identifier;
+        final Type valueType;
+        final V defaultValue;
+
+        Key(String identifier, Type valueType, V defaultValue) {
+            this.identifier = identifier;
+            this.valueType = valueType;
+            this.defaultValue = defaultValue;
+        }
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <V, S extends Setting<V>> S clone(S setting, V newValue) {
+        try {
+            setting.checkValue(newValue);
+        } catch (Exception e) {
+            throw new IllegalArgumentException("invalid value for key '"
+                    + ((Setting<V>)setting).key.identifier + "': " + ((Setting<V>)setting).value, e);
+        }
+
+        //important to clone so that we can retain checkValue(), toString() behavior on returned instance
+        S s = (S)setting.clone();
+
+        assert ((Setting<V>)s).key == ((Setting<V>)setting).key;
+        assert ((Setting<V>)s).getClass().equals(setting.getClass());
+
+        ((Setting<V>)s).value = newValue;
+        return s;
+    }
+
+    private static final Pattern identifierPattern = Pattern.compile("[a-z][0-9a-z]*(\\.[a-z][0-9a-z]*)*");
+    private static void checkIdentifier(String identifier) {
+        if (identifier == null) {
+            throw new IllegalArgumentException("identifier cannot be null");
+        }
+        if (!identifierPattern.matcher(identifier).matches()) {
+            throw new IllegalArgumentException("identifier does not match " + identifierPattern.pattern());
+        }
+    }
+
+    private static Type lookupValueType(Class<?> rawType, String identifier) {
+        HashMap<TypeVariable<?>, Type> mapping = new HashMap<>();
+        assert rawType != Setting.class;
+        for (;;) {
+            Type superclass = rawType.getGenericSuperclass();
+            if (superclass instanceof ParameterizedType) {
+                rawType = (Class)((ParameterizedType) superclass).getRawType();
+                Type[] args = ((ParameterizedType) superclass).getActualTypeArguments();
+                if (Setting.class.equals(rawType)) {
+                    Type type = args[0];
+                    type = mapping.getOrDefault(type, type);
+                    if (type instanceof Class) {
+                        if (((Class) type).isArray()) {
+                            throw new IllegalArgumentException(identifier + " value class must be immutable");
+                        } else if (((Class) type).getTypeParameters().length != 0) {
+                            throw new IllegalArgumentException(identifier + " setting key must fill in type parameters for " + ((Class) type).toGenericString());
+                        }
+                    } else if (type instanceof GenericArrayType) {
+                        throw new IllegalArgumentException(identifier + " value class must be immutable");
+                    } else if (type instanceof TypeVariable) {
+                        throw new IllegalArgumentException("Invalid setting key type 'Key<" + type.getTypeName() + ">' for identifier " + identifier);
+                    } else if (!(type instanceof ParameterizedType)) {
+                        throw new IllegalArgumentException(identifier + " invalid key type " + type + " (" + type.getClass().getName() + ")");
+                    }
+                    return type;
+                }
+                TypeVariable<?>[] vars = rawType.getTypeParameters();
+                for (int i = 0, len = vars.length; i < len; i++) {
+                    Type t = args[i];
+                    mapping.put(vars[i], t instanceof TypeVariable ? mapping.get(t) : t);
+                }
+            } else {
+                rawType = (Class<?>)superclass;
+                if (Setting.class.equals(rawType)) {
+                    throw new IllegalArgumentException(rawType + " does not supply type arguments");
+                }
+            }
+        }
+    }
+
 }
index 1289be3..9ef2e54 100644 (file)
@@ -44,10 +44,25 @@ public final class Settings extends AbstractSet<Setting<?>> {
     }
 
     /**
-     * Returns the setting with the same {@link Setting.Key Key} as the supplied setting, if present.
+     * @param identifier the identifier of the setting to find
+     * @return the setting with the identifier supplied, if present
      */
-    public <E> Optional<Setting<E>> find(Setting<E> setting) {
-        return setting.cast(values.get(setting.getIdentifier()));
+    public Optional<Setting<?>> find(String identifier) {
+        return Optional.ofNullable(values.get(identifier));
+    }
+
+    /**
+     * Returns the setting with the same setting key as the supplied setting, if present.
+     * <br><br>
+     * This method is semantically equivalent to:
+     * <br><br>
+     * <pre>
+     * {@code find(setting.getIdentifier()).flatMap(s -> s.as(setting))}
+     * </pre>
+     */
+    public <S extends Setting<?>> Optional<S> find(S setting) {
+        Setting<?> found = values.get(setting.getIdentifier());
+        return found == null ? Optional.empty() : found.as(setting);
     }
 
     /**
index a5a7b6e..4e799d4 100644 (file)
@@ -28,6 +28,7 @@ import java.util.Optional;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotSame;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
@@ -38,7 +39,7 @@ public class SettingsTest {
 
     @Test
     public void testNonNullSetting() {
-        Setting<String> nonNull = Setting.newKey("nulltest", String.class).withValue("A nonnull string");
+        Setting<String> nonNull = new Setting<String>("nulltest", "A nonnull string") {};
         try {
             nonNull.withValue(null);
             fail();
@@ -49,15 +50,15 @@ public class SettingsTest {
 
     @Test
     public void testNullableSetting() {
-        Setting<String> nullable = Setting.newKey("nulltest", String.class).withValue(null);
+        Setting<String> nullable = new Setting<String>("nulltest", null) {};
         assertNull(nullable.withValue(null).getValue());
     }
 
     @Test
     public void testDuplicateIdentifiers() {
         try {
-            Setting<String> first = Setting.newKey("foo", String.class).withValue("");
-            Setting<String> second = Setting.newKey("foo", String.class).withValue("");
+            Setting<String> first = new Setting<String>("foo", "") {};
+            Setting<String> second = new Setting<String>("foo", "") {};
 
             Settings.of(first, second);
 
@@ -69,7 +70,7 @@ public class SettingsTest {
 
     @Test
     public void testFind() {
-        Setting<String> key = Setting.newKey("foo", String.class).withValue("key");
+        Setting<String> key = new Setting<String>("foo", "key") {};
         Setting<String> element = key.withValue("element");
 
         Settings settings = Settings.of(element);
@@ -86,7 +87,7 @@ public class SettingsTest {
 
     @Test
     public void testGetPresentSetting() {
-        Setting<String> key = Setting.newKey("foo", String.class).withValue("key");
+        Setting<String> key = new Setting<String>("foo", "key") {};
 
         Setting<String> actual = key.withValue("actual");
         Settings settings = Settings.of(actual);
@@ -96,9 +97,9 @@ public class SettingsTest {
 
     @Test
     public void testGetAbsentSetting() {
-        Setting<String> key = Setting.newKey("foo", String.class).withValue("key");
+        Setting<String> key = new Setting<String>("foo", "key") {};
 
-        Setting<String> actual = Setting.newKey("foo", String.class).withValue("actual");
+        Setting<String> actual = new Setting<String>("foo", "actual") {};
         Settings settings = Settings.of(actual);
 
         assertSame(key.getValue(), settings.get(key));
@@ -106,26 +107,27 @@ public class SettingsTest {
 
     @Test
     public void testGetNullSetting() {
-        Setting.Key<String> baseKey = Setting.newKey("foo", String.class);
+        Setting<String> baseKey = new Setting<String>("foo", null) {};
 
-        Settings settings = Settings.of(baseKey.withValue(null));
+        Settings settings = Settings.of(baseKey);
         assertNull(settings.get(baseKey.withValue("not null")));
+
+        //make sure we can go back to null
+        baseKey.withValue("not null").withValue(null);
     }
 
     @Test
     public void testSettingType() {
-        assertEquals(CharSequence.class, Setting.newKey("foo", CharSequence.class).withValue("").getValueType());
-        assertEquals(CharSequence.class, new Setting.Key<CharSequence>("foo"){}.withValue("").getValueType());
+        assertEquals(CharSequence.class, new Setting<CharSequence>("foo", ""){}.getValueType());
 
-        Type mapType = new Setting.Key<Map<String, Integer>>(
-                "foo"){}.withValue(Collections.emptyMap()).getValueType();
+        Type mapType = new Setting<Map<String, Integer>>("foo", Collections.emptyMap()){}.getValueType();
 
         assertTrue(mapType instanceof ParameterizedType);
         assertEquals("java.util.Map<java.lang.String, java.lang.Integer>", mapType.getTypeName());
 
-        class Key0<Bar, V> extends Setting.Key<V> {
+        class Key0<Bar, V> extends Setting<V> {
             Key0() {
-                super("foo");
+                super("foo", null);
             }
         }
 
@@ -149,61 +151,77 @@ public class SettingsTest {
         assertEquals(String.class, simpleType);
     }
 
+    @Test
+    public void testCustomValueChecking() {
+        class PositiveIntegerSetting extends Setting<Integer> {
+            private PositiveIntegerSetting(String identifier, Integer defaultValue) {
+                super(identifier, defaultValue);
+            }
 
+            @Override
+            protected void checkValue(Integer newValue) throws Exception {
+                if (newValue < 0) {
+                    throw new NumberFormatException();
+                }
+            }
 
-    @Test
-    public void testBadSetting() {
-        try {
-            new Setting.Key("foo") {};
-            fail();
-        } catch (IllegalArgumentException e) {
-            //test passes; ignore
+            @Override
+            public String toString() {
+                return getValue().toString();
+            }
         }
 
-        try {
-            Setting.newKey("foo", null);
-            fail();
-        } catch (IllegalArgumentException e) {
-            //test passes; ignore
-        }
+        Setting<Integer> setting = new PositiveIntegerSetting("foo", 0);
+
+        assertNotSame(setting, setting.withValue(0));
+        assertEquals(setting, setting.withValue(0));
+
+        setting = setting.withValue(5).withValue(6).withValue(7);
+
+        assertEquals(setting.getClass(), PositiveIntegerSetting.class);
+        assertEquals(setting.toString(), "7");
 
         try {
-            Setting.newKey(null, Integer.class);
+            setting.withValue(-1);
             fail();
         } catch (IllegalArgumentException e) {
-            //test passes; ignore
+            assertTrue(e.getCause() instanceof NumberFormatException);
         }
+    }
 
+    @Test
+    @SuppressWarnings("unchecked")
+    public void testBadSetting() {
         try {
-            Setting.newKey(" ", Integer.class);
+            new Setting("foo", null) {};
             fail();
         } catch (IllegalArgumentException e) {
             //test passes; ignore
         }
 
         try {
-            Setting.newKey("foo", boolean.class);
+            new Setting<Integer>(null, null) {};
             fail();
         } catch (IllegalArgumentException e) {
             //test passes; ignore
         }
 
         try {
-            Setting.newKey("foo", Integer[].class);
+            new Setting<Integer>(" ", null) {};
             fail();
         } catch (IllegalArgumentException e) {
             //test passes; ignore
         }
 
         try {
-            new Setting.Key<Integer[]>("foo") {};
+            new Setting<Integer[]>("foo", null) {};
             fail();
         } catch (IllegalArgumentException e) {
             //test passes; ignore
         }
 
         try {
-            new Setting.Key<List<Integer>[]>("foo") {};
+            new Setting<List<Integer>[]>("foo", null) {};
             fail();
         } catch (IllegalArgumentException e) {
             //test passes; ignore
@@ -211,7 +229,7 @@ public class SettingsTest {
 
         class BadKeyCreator {
             private <V> void badKey() {
-                new Setting.Key<V>("foo") {};
+                new Setting<V>("foo", null) {};
             }
         }
 
index ef912f7..c2f0557 100644 (file)
@@ -77,9 +77,8 @@ public class Rover extends BaseTool {
     private static final String DEFAULT_WRITER_IDENTIFIER = NTriplesWriterFactory.IDENTIFIER;
 
     static {
-        final Setting<Boolean> ALWAYS_SUPPRESS_CSS_TRIPLES = Setting.newKey(
-                "alwayssuppresscsstriples", Boolean.class)
-                .withValue(Boolean.TRUE);
+        final Setting<Boolean> ALWAYS_SUPPRESS_CSS_TRIPLES = new Setting<Boolean>(
+                "alwayssuppresscsstriples", Boolean.TRUE) {};
         final Settings supportedSettings = Settings.of(ALWAYS_SUPPRESS_CSS_TRIPLES);
 
         registry.register(new DecoratingWriterFactory() {
index 40e3b26..44094ad 100644 (file)
@@ -46,14 +46,12 @@ public class WriterSettings {
     /**
      * Directive to writer that output should be printed in a way to maximize human readability.
      */
-    public static final Setting<Boolean> PRETTY_PRINT = Setting.newKey("pretty", Boolean.class)
-            .withValue(Boolean.TRUE);
+    public static final Setting<Boolean> PRETTY_PRINT = new Setting<Boolean>("pretty", Boolean.TRUE){};
 
     /**
      * Directive to writer that at least the non-ASCII characters should be escaped.
      */
-    public static final Setting<Boolean> PRINT_ASCII = Setting.newKey("ascii", Boolean.class)
-            .withValue(Boolean.FALSE);
+    public static final Setting<Boolean> PRINT_ASCII = new Setting<Boolean>("ascii", Boolean.FALSE){};
 
 
 }