Code review comment for lp:~mbp/bzr/deprecation-old

Martin Pool (mbp) wrote :

2009/6/8 John A Meinel <email address hidden>:
> Review: Approve
> This seems okay, though I wonder if you deleted too many tests.
> Usually when we deprecate something, we keep it under test, and add a "callDeprecated" or "applyDeprecated" wrap around its call. Which ensures that
> 1) it is still properly deprecated
> 2) And it still works the way it used to, until it is removed
> Deleting the tests of things that were already deprecated is, of course, fine.
> Anyway, just take another quick look and see if you really wanted to delete all of TestProgressTypes, and if so, go ahead and merge.

I wondered whether we should delete them too, however, keeping them
and getting them to catch warnings in all the right places was just
more work than seemed worthwhile for code that's going away. It also
makes it more clear where testing is needed for the new code. :)

The specific problem was that although it's easy to test that one
particular deprecated function works, there's no obvious way to say
"and it's ok if it calls other deprecated code in turn." Really we
also shouldn't issue warnings in that case even at run time. Some
fixes are possible but I didn't want to do them here.

Martin <>

« Back to merge proposal