launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

Review: Approve

Jonathan--

This looks good. I have two quibbles about comments in the code below, but that's all.

> === modified file 'lib/lp/services/tests/test_utils.py'
> --- lib/lp/services/tests/test_utils.py	2011-12-19 23:38:16 +0000
> +++ lib/lp/services/tests/test_utils.py	2012-05-31 12:14:21 +0000
> @@ -384,3 +388,14 @@
>          """Values are obfuscated recursively."""
>          obfuscated = obfuscate_structure({'foo': (['a@xxxxxxxxxxx'],)})
>          self.assertEqual({'foo': [['<email address hidden>']]}, obfuscated)
> +
> +
> +class TestTotalSeconds(TestCase):
> +
> +    # Remove this when Python 2.6 support is dropped.  Replace calls with
> +    # timedelta.total_seconds.
> +
> +    def test_total_seconds(self):
> +        # Numbers are arbitrary.
> +        duration = timedelta(days=3, seconds=45, microseconds=16)
> +        self.assertEqual(3 * 24 * 3600 + 45.000016, total_seconds(duration))
 
Could you make the comment in this test an XXX comment? It marginally
increases the chances other people will notice this and kill it appropriately.
 
> === modified file 'lib/lp/services/utils.py'
> --- lib/lp/services/utils.py	2012-05-24 20:25:54 +0000
> +++ lib/lp/services/utils.py	2012-05-31 12:14:21 +0000
> @@ -26,6 +26,7 @@
>      'save_bz2_pickle',
>      'synchronize',
>      'text_delta',
> +    'total_seconds',
>      'traceback_info',
>      'utc_now',
>      'value_string',
> @@ -383,3 +384,15 @@
>              for key, value in o.iteritems())
>      else:
>          return o
> +
> +
> +def total_seconds(duration):
> +    """The number of total seconds in a timedelta.
> +
> +    In Python 2.7, spell this as duration.total_seconds().  Only needed for
> +    Python 2.6 or earlier.
> +    """
> +    return (
> +        (duration.microseconds +
> +         (duration.seconds + duration.days * 24 * 3600) * 1e6)
> +        / 1e6)
 
This should probably have a similar XXX comment.

I believe I mentioned the LoC tool in IRC; as you mentioned, this is part of a broader arc of work--if that's going to end up reducing LoC, fantastic. If not, you still have that credit I mentioned, so either way this should be fine as regards our LoC limits.

Thanks!
-- 
https://code.launchpad.net/~jml/launchpad/generate-ppa-logging/+merge/108040
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.