Repository: any23 Updated Branches: refs/heads/master d97538fdb -> bf14ff4fc
simplify Settings API Project: http://git-wip-us.apache.org/repos/asf/any23/repo Commit: http://git-wip-us.apache.org/repos/asf/any23/commit/bf14ff4f Tree: http://git-wip-us.apache.org/repos/asf/any23/tree/bf14ff4f Diff: http://git-wip-us.apache.org/repos/asf/any23/diff/bf14ff4f Branch: refs/heads/master Commit: bf14ff4fc15170ce31eea54e3557d0b40886490d Parents: d97538f Author: Hans <[email protected]> Authored: Fri Oct 26 16:11:42 2018 -0500 Committer: Hans <[email protected]> Committed: Fri Oct 26 16:11:42 2018 -0500 ---------------------------------------------------------------------- .../org/apache/any23/configuration/Setting.java | 328 +++++++++---------- .../apache/any23/configuration/Settings.java | 21 +- .../any23/configuration/SettingsTest.java | 92 +++--- .../main/java/org/apache/any23/cli/Rover.java | 5 +- .../org/apache/any23/writer/WriterSettings.java | 6 +- 5 files changed, 224 insertions(+), 228 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/any23/blob/bf14ff4f/api/src/main/java/org/apache/any23/configuration/Setting.java ---------------------------------------------------------------------- diff --git a/api/src/main/java/org/apache/any23/configuration/Setting.java b/api/src/main/java/org/apache/any23/configuration/Setting.java index 92a3632..25f07f8 100644 --- a/api/src/main/java/org/apache/any23/configuration/Setting.java +++ b/api/src/main/java/org/apache/any23/configuration/Setting.java @@ -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 ([email protected]) */ -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"); + } + } + } + } + } http://git-wip-us.apache.org/repos/asf/any23/blob/bf14ff4f/api/src/main/java/org/apache/any23/configuration/Settings.java ---------------------------------------------------------------------- diff --git a/api/src/main/java/org/apache/any23/configuration/Settings.java b/api/src/main/java/org/apache/any23/configuration/Settings.java index 1289be3..9ef2e54 100644 --- a/api/src/main/java/org/apache/any23/configuration/Settings.java +++ b/api/src/main/java/org/apache/any23/configuration/Settings.java @@ -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); } /** http://git-wip-us.apache.org/repos/asf/any23/blob/bf14ff4f/api/src/test/java/org/apache/any23/configuration/SettingsTest.java ---------------------------------------------------------------------- diff --git a/api/src/test/java/org/apache/any23/configuration/SettingsTest.java b/api/src/test/java/org/apache/any23/configuration/SettingsTest.java index a5a7b6e..4e799d4 100644 --- a/api/src/test/java/org/apache/any23/configuration/SettingsTest.java +++ b/api/src/test/java/org/apache/any23/configuration/SettingsTest.java @@ -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) {}; } } http://git-wip-us.apache.org/repos/asf/any23/blob/bf14ff4f/cli/src/main/java/org/apache/any23/cli/Rover.java ---------------------------------------------------------------------- diff --git a/cli/src/main/java/org/apache/any23/cli/Rover.java b/cli/src/main/java/org/apache/any23/cli/Rover.java index ef912f7..c2f0557 100644 --- a/cli/src/main/java/org/apache/any23/cli/Rover.java +++ b/cli/src/main/java/org/apache/any23/cli/Rover.java @@ -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() { http://git-wip-us.apache.org/repos/asf/any23/blob/bf14ff4f/core/src/main/java/org/apache/any23/writer/WriterSettings.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/any23/writer/WriterSettings.java b/core/src/main/java/org/apache/any23/writer/WriterSettings.java index 40e3b26..44094ad 100644 --- a/core/src/main/java/org/apache/any23/writer/WriterSettings.java +++ b/core/src/main/java/org/apache/any23/writer/WriterSettings.java @@ -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){}; }
