One other thing I was wondering about is whether I should have deprecated the setXXX(String) methods that I removed. I've noticed past API changes that might have deprecated methods but didn't (generally renaming methods). Obviously it would be less imposing on users to keep the deprecated methods for at least a single release.
I can back out the change & deprecate the methods if preferred. Chris On 15 September 2010 19:14, Chris Bartlett <[email protected]> wrote: > Greg, > > Replies below > > Chris > > On 15 September 2010 18:38, Greg Brown <[email protected]> wrote: > >> Hi Chris, >> >> Looks good - thanks for tackling this one. Comments below: >> >> > Modified: pivot/trunk/core/src/org/apache/pivot/beans/BeanAdapter.java >> > @@ -841,6 +844,8 @@ public class BeanAdapter implements Map< >> > if (type.isAssignableFrom(value.getClass())) { >> > // Value doesn't need coercion >> > coercedValue = value; >> > + } else if (type.isEnum() && value instanceof String) { >> > + coercedValue = coerceEnum(value, type); >> >> Since this is a coercion method, it might be more flexible to check for >> value != null and call toString() on it. >> >> OK, will do. > > > + @SuppressWarnings("unchecked") >> > + private static <T> T coerceEnum(Object value, Class<? extends T> >> type) { >> >> Was there a reason you put this in a separate method? I imagined we'd just >> inline it in the coerce method. >> > I was thinking it might be useful in its own right & the logic is different > enough from the primitives coercion. > If you'd rather have it inline, I can make the change. > > >> > + } >> > + if (!(value instanceof String)) { >> > + throw new IllegalArgumentException(String.format( >> > + "Non-null String value must be supplied for enum >> coercion. Value=%s", >> > + (value == null ? null : value.getClass().getName()))); >> > + } >> >> Same comment here as above - it would probably be more flexible to call >> toString() rather than cast to String. Also, null is a valid enum value, so >> we should allow it. >> >> Will make the change. > > >> > + >> > + // Find and invoke the valueOf(String) method, with an upper >> case >> > + // version of the supplied String >> > + T coercedValue = null; >> > + try { >> > + Method valueOfMethod = >> type.getMethod(ENUM_VALUE_OF_METHOD_NAME, String.class); >> > + if (valueOfMethod != null) { >> > + String valueString = ((String) >> value).toUpperCase(Locale.ENGLISH); >> > + coercedValue = (T) valueOfMethod.invoke(null, >> valueString); >> > + } >> > + } >> > + // Nothing to be gained by handling the getMethod() & invoke() >> > + // Exceptions separately >> > + catch (IllegalAccessException e) { >> > + throwCoerceEnumException(value, type, e); >> >> In general, we don't define "thrower" methods. You could instead define a >> constant for your exception message and re-use that in each catch block. >> Later, when we (hopefully) get multi-catch in JDK 7, we can consolidate >> blocks like this. >> >> Just trying to reduce code repetition, but I know it is clunky. > I should have removed it earlier actually, as I simplified the exception > message so it can just as easily be created using a constant as you > suggested. I blame delays between writing, testing and checking in for not > catching it. :) > > >
