Robert Thanks for taking the time to look at this and giving feedback. I take the hint about the test cases - I'll have to get into how its done in jakarta some time.
On point 3, I agree about fixing the exception handling in the right place, rather than in this specific instance. The other point about this change was a performance improvement. When the WrapDyanClass is constructed it introspects to discover and store the PropertyDescriptors for the bean, but WrapDyanBean uses the get/setSimpleProperty methods in PropertyUtilsBean which means this introspection is repeated for every get/set. This seems a shame and must be faster just to introspect once . Maybe the answer is to split PropertyUtilsBean get/setSimpleProperty methods each into two methods so that WrapDyanBean could take advantage of the stored PropertyDescriptors while leaving PropertyUtilsBean processing unchanged. I include example setSimpleProperty methods below. Anyway 2 out of 3 would be a good result for me, I'd rather use as close to a "vanilla" flavor of beanutils as possible - so anthing that gets into the code base is great. Niall =========>Example: public void setSimpleProperty(Object bean, String name, Object value) throws IllegalAccessException, InvocationTargetException, NoSuchMethodException { setSimpleProperty(bean, name, value, null); } public void setSimpleProperty(Object bean, String name, Object value, PropertyDescriptor propertyDescriptor) throws IllegalAccessException, InvocationTargetException, NoSuchMethodException { if (bean == null) { throw new IllegalArgumentException("No bean specified"); } if (name == null) { throw new IllegalArgumentException("No name specified"); } // Validate the syntax of the property name if (name.indexOf(PropertyUtils.NESTED_DELIM) >= 0) { throw new IllegalArgumentException ("Nested property names are not allowed"); } else if (name.indexOf(PropertyUtils.INDEXED_DELIM) >= 0) { throw new IllegalArgumentException ("Indexed property names are not allowed"); } else if (name.indexOf(PropertyUtils.MAPPED_DELIM) >= 0) { throw new IllegalArgumentException ("Mapped property names are not allowed"); } // Handle DynaBean instances specially if (bean instanceof DynaBean) { DynaProperty descriptor = ((DynaBean) bean).getDynaClass().getDynaProperty(name); if (descriptor == null) { throw new NoSuchMethodException("Unknown property '" + name + "'"); } ((DynaBean) bean).set(name, value); return; } // Retrieve the property setter method for the specified property PropertyDescriptor descriptor = propertyDescriptor == null ? getPropertyDescriptor(bean, name) : propertyDescriptor; if (descriptor == null) { throw new NoSuchMethodException("Unknown property '" + name + "'"); } Method writeMethod = getWriteMethod(descriptor); if (writeMethod == null) { throw new NoSuchMethodException("Property '" + name + "' has no setter method"); } // Call the property setter method Object values[] = new Object[1]; values[0] = value; if (log.isTraceEnabled()) { String valueClassName = value == null ? "<null>" : value.getClass().getName(); log.trace("setSimpleProperty: Invoking method " + writeMethod + " with value " + value + " (class " + valueClassName + ")"); } invokeMethod(writeMethod, bean, values); } ----- Original Message ----- From: "robert burrell donkin" <[EMAIL PROTECTED]> To: "Jakarta Commons Developers List" <[EMAIL PROTECTED]> Sent: Tuesday, January 06, 2004 8:32 PM Subject: Re: [BeanUtils] WrapDynaBean Enhacement Request > hi niall > > patches are much more likely to get promptly applied if they come with > unit tests (hint, hint ;) > > 1+2 it seemed clear that craig intended no constructors to be available > (and he usually has good reasons for his design decisions) but i think > that allowing WrapDynaClass and WrapDynaBean to fit better into > DynaBean frameworks seems like a strong argument for providing this > functionality. unless someone else speaks up, i'll likely commit > something along these lines. > > 3 seems to me to be a symptom of a bigger issue (which has been known > for some time). the exception handling in beanutils is painful and > confusing to many users. i'd be very reluctant to break backwards > compatibility (even for symantics) and i suspect that the proposed > patch does. we've talked before about the possibility of factoring out > the exception handling and possible it might be better to fix this > rather than the symptom. so maybe a little more talk and thought is > needed about this one. > > comments welcome > > - robert > > On 6 Jan 2004, at 00:17, Niall Pemberton wrote: > > > Thanks, feedback would be much appreciated. > > > > Niall > > ----- Original Message ----- > > From: "robert burrell donkin" <[EMAIL PROTECTED]> > > To: "Jakarta Commons Developers List" <[EMAIL PROTECTED]> > > Sent: Monday, January 05, 2004 10:44 PM > > Subject: Re: [BeanUtils] WrapDynaBean Enhacement Request > > > > > >> hi niall > >> > >> i'll take a look at this sometime soonish. > >> > >> - robert > >> > >> On 30 Dec 2003, at 13:14, Niall Pemberton wrote: > >> > >>> I submitted a bugzilla a while ago to enhace > >>> WrapDynaBean/WrapDynaClass: > >>> > >>> http://nagoya.apache.org/bugzilla/show_bug.cgi?id=23690 > >>> > >>> > >>> The changes were > >>> > >>> 1) Implement a getInstance() method in WrapDynaBean so that the > >>> "original" wrapped bean can be retrieved. > >>> 2) Implement a newInstance() method in WrapDynaClass to generate new > >>> "wrapped" beans. > >>> 3) Change the get(name) and set(name, value) methods to make them > >>> more > >>> efficient and provide more usefull exception messages. > >>> > >>> > >>> Is there any chance on getting some feedback and/or indication > >>> whether > >>> there is any interest in applying it? > >>> > >>> Thanks > >>> > >>> Niall > >> > >> > >> --------------------------------------------------------------------- > >> To unsubscribe, e-mail: [EMAIL PROTECTED] > >> For additional commands, e-mail: [EMAIL PROTECTED] > >> > >> > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: [EMAIL PROTECTED] > > For additional commands, e-mail: [EMAIL PROTECTED] > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]