On 22/01/2009, Niall Pemberton <niall.pember...@gmail.com> wrote: > On Wed, Jan 21, 2009 at 11:16 AM, Phil Steitz <phil.ste...@gmail.com> wrote: > > sebb wrote: > >> > >> On 20/01/2009, Phil Steitz <phil.ste...@gmail.com> wrote: > >> > >>> > >>> I would like to ask that these changes be reverted. Deprecated classes > >>> and > >>> client code breakage that depends on them can only happen in major (x.0) > >>> releases. Unless I am missing something subtle, these changes will break > >>> client code that specializes the cast on caught exceptions and tries to > >>> unpack the nested root cause exception. > >>> > >> > >> Throwable.getCause() was added in 1.4, and DBCP now targets 1.5+ > >> (according to the Maven build files) so there won't be a problem > >> calling getCause(), however the cast to SQLNestedException would fail. > >> > >> > >>> > >>> Since this is a .y release, we > >>> should not make this change at this point. > >>> > >> > >> OK, I was a bit hasty (though I did ask first, I did not wait long > >> enough). > >> > > > > Sorry I took so long to review. > >> > >> Rather than remove the new code, how about changing: > >> > >> throw (SQLException) new SQLException(message).initCause(e); > >> > >> into: > >> > >> throw (SQLException) new SQLNestedException(message).setCause(e); > >> > >> The SQLNestedException class would need to be extended to add a > >> (String) constructor and a setCause() method which copies the relevant > >> code from the existing constructor. > >> > >> That is easy to do now and easy to undo when the time comes to remove > >> the deprecated class. I think it maintains full compatibility as well. > >> > >> [If overriding Throwable.initCause() is allowed, then the change could > >> be simplified slightly] > >> > >> WDYT? > >> > > > > Honestly, I think the best solution is to revert the change. When, if > ever, > > DBCP sees a 2.0, we can clean remove SQLNestedException and clean up > > systematically. I don't see the risk of other unanticipated impacts as > > being worth the simplification in cleanup later. We would also have to add > > test cases. So my vote is for less work and breakage risk now, which > means > > revert the change. > > > +1
OK, done. I kept the other change from 734470, which was the removal of an impossible null check. > Niall > > > > Phil > >>> > >>> Phil > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
