Hi Niall, I completely missed the mail, so sorry for responding so late and thanx for your extensive reply ! After some thought about this, I came to a couple of insights :)
- You know the consequences of the change you made to beanutils a lot better than I do, so I leave that responsibility to you. Although I would like to see some form of documentation about this specific case (cannot imagine that betwixt is the only one that will run into this problem). The problem is that the testcode in betwixt looks buggy or at least weird to some extend, so maybe it's a hard case to explain. - The consequence I see now is that people if I adhere to the new way convert works, there will be problems with the currently released versions of beanutils, so : I decided to leave it to the user to use the improved beanutils functionality and solved the build problem by copying the beanutils 1.7 code over to betwixt (see http://svn.apache.org/viewvc?view=rev&revision=552049). This gives me the certainty that people don't have any problems "deserliazing" their objects when beanutils trunk is on their classpath :) Don't know if your changes still serve a purpose, if not, you should be able to revert it without breanking betwixt. Mvgr, Martin Niall Pemberton wrote: > On 6/23/07, Martin van den Bemt <[EMAIL PROTECTED]> wrote: >> Noticed the call of toString() on a String during the huntdown of what >> in beanutils broke the >> betwixt tests. (in the TestObjectStringConverters) >> The commit was a bit premature probably, although this is most (read >> most, so not all) of the time >> faster that calling toString() on a String. Will revert it (after some >> sleep) if that is what you >> are asking (code is more readable without the addition, agreed). >> >> Another questions (probably related to BEANUTILS-258) : The failing >> gump of betwixt is related to >> the changes you made to ConvertUtilsBean.convert(Object). In beanutils >> 1.7 a default lookup is done >> for the type String.class and in the new code this is just the case >> when no converter can be found >> for the sourcetype, which makes the new beanutils code not a drop in >> replacement of the old one and >> not backward compatible. I will see if I can run the beanutils 1.7 >> testcases against trunk tomorrow >> (they should pass, or am I being simplistic here?) >> >> Was this breakage intended and what are your thoughts on how to handle >> this ? > > I only see 6 failures in 2 test cases (and looking at the gump output > seems the same) and really its only 3 because one test case extends > the other and its the same failures. > > TestObjectStringConverters (3 failures) > - testConvertUtilsConverter > - testDefaultOSConverter > - testDefaultOSConverterDates > > Testi18nObjectStringConversion (3 failures) > (same failures as above since it inherits from TestObjectStringConverters) > > The cause is the same in all cases - these tests are effectively > testing that ConvertUtils is delegating to the Converter registered > for the String.class. This is what I (intentionally) changed. > > http://issues.apache.org/jira/browse/BEANUTILS-258 > > The contract for BeanUtils's Converter includes the "type" you want to > convert to. Unfortunately the Converter implementations mostly ignored > this and for conversion to String ConvertUtils delegated to the > registered String Converter. Makes more sense to me if the Converter > registered for the "Type" handled conversion from the type to String. > So for example - the date Converter implementations can now be > configured with a pattern (& Locale) and use that pattern to convert > from String to Date and from Date to String. The same is true for the > improved number converters (which can now also be configured with > patterns and a Locale). > > In light of the betwixt issue I have made one change to the > ConvertUtilsBean's new convert() method - if the registered Converter > doesn't convert the value to a String it tries the registered String > Conveter first - before then defaulting to the object's toString() > method. This only partially resolves the compatability issue though > and doesn't stop Betwixt tests failing. > > http://svn.apache.org/viewvc?view=rev&revision=551047 > > On the compatibility issue - I believe the Converter implementations > provided by BeanUtils are backwards compatible and its only where > people have registered their own implementations that there may be > issues. With my change today - if their Converter implementations > don't handle conversion to String then it will continue to delegate to > the registered String Converter - if their Converter doesn't cope well > with that then they have an issue. The solution is fairly straight > forward though since I have added a new AbstractConverter > implementation that provides a structure to easily add String > conversion capability. The question is though whether this is all > enough (IMO yes). > > If not then the one option would for the existing ConvertUtilsBean's > convert() methods to revert to their previour behaviour (and be > deprecated?) and not delegate to the new convert() method impl. This > would resolve the issue for Betwixt (which uses > ConvertUtils/ConvertUtilsBean) - but not for BeanUtils/BeanUtilsBean - > whose methods (setProperty/copyProperty) should IMO use the new > features. > > Another option would be to reinstate backward compatible behaviour > with a configuration option (not sure what default would be) to switch > on/off the new features. Personally I'm not keen on doing that work > but if that is the preferred route I would hope the default behaviour > would be the new, rather than old behaviour. > > Opinions? > > Niall > > P.S. If the consensus is to leave BeanUtils as it is - the Betwixt > tests can be made to work with both the current BeanUtils trunk and > the previous (1.7.0) release with only minor modifications, which I > can do. > >> Mvgr, >> Martin >> >> Niall Pemberton wrote: >> > Is there a reason for this change? AFAIK calling toString() on a >> > String object just returns a reference to itself - so this just seems >> > to add clutter in my mind. Also there was discussion on this (i.e. >> > calling toString() on a String) for this very bit of code in the >> > following issue ticket - would have been nice to comment before >> > arbitarily making the change >> > >> > http://issues.apache.org/jira/browse/BEANUTILS-283 >> > >> > Niall >> > >> > On 6/23/07, [EMAIL PROTECTED] <[EMAIL PROTECTED]> wrote: >> >> Author: mvdb >> >> Date: Fri Jun 22 17:58:02 2007 >> >> New Revision: 549986 >> >> >> >> URL: http://svn.apache.org/viewvc?view=rev&rev=549986 >> >> Log: >> >> Prevent calling toString on a String. >> >> >> >> Modified: >> >> >> >> >> jakarta/commons/proper/beanutils/trunk/src/java/org/apache/commons/beanutils/ConvertUtilsBean.java >> >> >> >> >> >> >> Modified: >> >> >> jakarta/commons/proper/beanutils/trunk/src/java/org/apache/commons/beanutils/ConvertUtilsBean.java >> >> >> >> >> URL: >> >> >> http://svn.apache.org/viewvc/jakarta/commons/proper/beanutils/trunk/src/java/org/apache/commons/beanutils/ConvertUtilsBean.java?view=diff&rev=549986&r1=549985&r2=549986 >> >> >> >> >> >> ============================================================================== >> >> >> >> >> --- >> >> >> jakarta/commons/proper/beanutils/trunk/src/java/org/apache/commons/beanutils/ConvertUtilsBean.java >> >> >> (original) >> >> +++ >> >> >> jakarta/commons/proper/beanutils/trunk/src/java/org/apache/commons/beanutils/ConvertUtilsBean.java >> >> >> Fri Jun 22 17:58:02 2007 >> >> @@ -542,7 +542,8 @@ >> >> } >> >> converted = converter.convert(targetType, value); >> >> } >> >> - if (targetType == String.class && converted != null) { >> >> + if (targetType == String.class && converted != null && >> >> + !(converted instanceof String)) { >> >> converted = converted.toString(); >> >> } >> >> return converted; > > --------------------------------------------------------------------- > 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]
