You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pivot.apache.org by Greg Brown <gk...@mac.com> on 2010/09/15 13:38:17 UTC
Re: svn commit: r997274 - in /pivot/trunk:
core/src/org/apache/pivot/beans/BeanAdapter.java
tests/src/org/apache/pivot/tests/EnumBean.java
tests/src/org/apache/pivot/tests/EnumBeanTest.java
tests/src/org/apache/pivot/tests/enum_bean.bxml
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.
> + @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.
> + }
> + 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.
> +
> + // 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.
Greg
Re: svn commit: r997274 - in /pivot/trunk: core/src/org/apache/pivot/beans/BeanAdapter.java
tests/src/org/apache/pivot/tests/EnumBean.java tests/src/org/apache/pivot/tests/EnumBeanTest.java
tests/src/org/apache/pivot/tests/enum_bean.bxml
Posted by Chris Bartlett <cb...@gmail.com>.
Changes made in r997330
On 15 September 2010 19:35, Greg Brown <gk...@mac.com> wrote:
> >>> + 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.
>
> Since the same functionality is available by calling coerce() directly, I
> think it would be best to simply inline it there.
>
>
>
Re: svn commit: r997274 - in /pivot/trunk:
core/src/org/apache/pivot/beans/BeanAdapter.java
tests/src/org/apache/pivot/tests/EnumBean.java
tests/src/org/apache/pivot/tests/EnumBeanTest.java
tests/src/org/apache/pivot/tests/enum_bean.bxml
Posted by Greg Brown <gk...@mac.com>.
>>> + 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.
Since the same functionality is available by calling coerce() directly, I think it would be best to simply inline it there.
Re: svn commit: r997274 - in /pivot/trunk:
core/src/org/apache/pivot/beans/BeanAdapter.java
tests/src/org/apache/pivot/tests/EnumBean.java
tests/src/org/apache/pivot/tests/EnumBeanTest.java
tests/src/org/apache/pivot/tests/enum_bean.bxml
Posted by Greg Brown <gk...@mac.com>.
> One other thing I was wondering about is whether I should have deprecated
> the setXXX(String) methods that I removed.
If we made this change between 1.5 and a hypothetical 1.6 release, it might make sense to deprecate them. However, since we're moving to 2.0, I think it is OK to simply remove them (we expect to see major API changes across major releases).
Re: svn commit: r997274 - in /pivot/trunk: core/src/org/apache/pivot/beans/BeanAdapter.java
tests/src/org/apache/pivot/tests/EnumBean.java tests/src/org/apache/pivot/tests/EnumBeanTest.java
tests/src/org/apache/pivot/tests/enum_bean.bxml
Posted by Chris Bartlett <cb...@gmail.com>.
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 <cb...@gmail.com> wrote:
> Greg,
>
> Replies below
>
> Chris
>
> On 15 September 2010 18:38, Greg Brown <gk...@mac.com> 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. :)
>
>
>
Re: svn commit: r997274 - in /pivot/trunk: core/src/org/apache/pivot/beans/BeanAdapter.java
tests/src/org/apache/pivot/tests/EnumBean.java tests/src/org/apache/pivot/tests/EnumBeanTest.java
tests/src/org/apache/pivot/tests/enum_bean.bxml
Posted by Chris Bartlett <cb...@gmail.com>.
Greg,
Replies below
Chris
On 15 September 2010 18:38, Greg Brown <gk...@mac.com> 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. :)